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@nvidia.com>
---
v1 -> v2:
	* Use dev_err_probe() in tegra_utc_probe().
	* Use uart_read_port_properties() instead of manually parsing
	  the port line.
	* Remove duplicate error prints if platform_get_irq() fails.
	* In tegra_utc_of_match, remove `,` after terminator line.
	* Remove current-speed, as it is not always accurate.
---
 drivers/tty/serial/Kconfig     |  23 ++
 drivers/tty/serial/Makefile    |   1 +
 drivers/tty/serial/tegra-utc.c | 622 +++++++++++++++++++++++++++++++++
 3 files changed, 646 insertions(+)
 create mode 100644 drivers/tty/serial/tegra-utc.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 976dae3bb1bb..edc56a3c0ace 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -306,6 +306,29 @@ config SERIAL_TEGRA_TCU_CONSOLE
 
 	  If unsure, say Y.
 
+config SERIAL_TEGRA_UTC
+	tristate "NVIDIA Tegra UART Trace Controller"
+	depends on ARCH_TEGRA || COMPILE_TEST
+	select SERIAL_CORE
+	help
+	  Support for Tegra UTC (UART Trace controller) client serial port.
+
+	  UTC is a HW based serial port that allows multiplexing multiple data
+	  streams of up to 16 UTC clients into a single hardware serial port.
+
+config SERIAL_TEGRA_UTC_CONSOLE
+	bool "Support for console on a Tegra UTC serial port"
+	depends on SERIAL_TEGRA_UTC
+	select SERIAL_CORE_CONSOLE
+	default SERIAL_TEGRA_UTC
+	help
+	  If you say Y here, it will be possible to use a Tegra UTC client as
+	  the system console (the system console is the device which receives
+	  all kernel messages and warnings and which allows logins in single
+	  user mode).
+
+	  If unsure, say Y.
+
 config SERIAL_MAX3100
 	tristate "MAX3100/3110/3111/3222 support"
 	depends on SPI
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 6ff74f0a9530..7190914ba707 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_SERIAL_STM32)		+= stm32-usart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)		+= sunplus-uart.o
 obj-$(CONFIG_SERIAL_TEGRA)		+= serial-tegra.o
 obj-$(CONFIG_SERIAL_TEGRA_TCU)		+= tegra-tcu.o
+obj-$(CONFIG_SERIAL_TEGRA_UTC)		+= tegra-utc.o
 obj-$(CONFIG_SERIAL_TIMBERDALE)		+= timbuart.o
 obj-$(CONFIG_SERIAL_TXX9)		+= serial_txx9.o
 obj-$(CONFIG_SERIAL_UARTLITE)		+= uartlite.o
diff --git a/drivers/tty/serial/tegra-utc.c b/drivers/tty/serial/tegra-utc.c
new file mode 100644
index 000000000000..75d39cb8394a
--- /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>
+#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>
+#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)
+
+#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;
+};
+
+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;
+};
+
+static u32 tegra_utc_rx_readl(struct tegra_utc_port *tup, unsigned int offset)
+{
+	void __iomem *addr = tup->rx_base + offset;
+
+	return readl_relaxed(addr);
+}
+
+static void tegra_utc_rx_writel(struct tegra_utc_port *tup, u32 val, unsigned int offset)
+{
+	void __iomem *addr = tup->rx_base + offset;
+
+	writel_relaxed(val, addr);
+}
+
+static u32 tegra_utc_tx_readl(struct tegra_utc_port *tup, unsigned int offset)
+{
+	void __iomem *addr = tup->tx_base + offset;
+
+	return readl_relaxed(addr);
+}
+
+static void tegra_utc_tx_writel(struct tegra_utc_port *tup, u32 val, unsigned int offset)
+{
+	void __iomem *addr = tup->tx_base + offset;
+
+	writel_relaxed(val, addr);
+}
+
+static void tegra_utc_enable_tx_irq(struct tegra_utc_port *tup)
+{
+	tup->tx_irqmask = TEGRA_UTC_INTR_REQ;
+
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_disable_tx_irq(struct tegra_utc_port *tup)
+{
+	tup->tx_irqmask = 0x0;
+
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_stop_tx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_disable_tx_irq(tup);
+}
+
+static void tegra_utc_init_tx(struct tegra_utc_port *tup)
+{
+	/* Disable TX. */
+	tegra_utc_tx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
+
+	/* Update the FIFO Threshold. */
+	tegra_utc_tx_writel(tup, tup->tx_threshold, TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear and mask all the interrupts. */
+	tegra_utc_tx_writel(tup, TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
+			    TEGRA_UTC_INTR_CLEAR);
+	tegra_utc_disable_tx_irq(tup);
+
+	/* Enable TX. */
+	tegra_utc_tx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
+}
+
+static void tegra_utc_init_rx(struct tegra_utc_port *tup)
+{
+	tup->rx_irqmask = TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_TIMEOUT;
+
+	tegra_utc_rx_writel(tup, TEGRA_UTC_COMMAND_RESET, TEGRA_UTC_COMMAND);
+	tegra_utc_rx_writel(tup, tup->rx_threshold, TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear all the pending interrupts. */
+	tegra_utc_rx_writel(tup, TEGRA_UTC_INTR_TIMEOUT | TEGRA_UTC_INTR_OVERFLOW |
+			    TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL |
+			    TEGRA_UTC_INTR_EMPTY, TEGRA_UTC_INTR_CLEAR);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_SET);
+
+	/* Enable RX. */
+	tegra_utc_rx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
+}
+
+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)
+{
+	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;
+	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);
+		uart_port_lock(&tup->port);
+
+		if (!sysrq)
+			tty_insert_flip_char(port, ch, flag);
+	}
+
+	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;
+}
+
+static unsigned int tegra_utc_tx_empty(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	return tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_OCCUPANCY) ? 0 : TIOCSER_TEMT;
+}
+
+static void tegra_utc_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static unsigned int tegra_utc_get_mctrl(struct uart_port *port)
+{
+	return 0;
+}
+
+static void tegra_utc_start_tx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	if (tegra_utc_tx_chars(tup))
+		tegra_utc_enable_tx_irq(tup);
+}
+
+static void tegra_utc_stop_rx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tup->rx_irqmask = 0x0;
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_hw_init(struct tegra_utc_port *tup)
+{
+	tegra_utc_init_tx(tup);
+	tegra_utc_init_rx(tup);
+}
+
+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);
+	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);
+	free_irq(tup->irq, tup);
+}
+
+static void tegra_utc_set_termios(struct uart_port *port, struct ktermios *termios,
+				  const struct ktermios *old)
+{
+	/* The Tegra UTC clients supports only 8-N-1 configuration without HW flow control */
+	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD);
+	termios->c_cflag &= ~(CMSPAR | CRTSCTS);
+	termios->c_cflag |= CS8 | CLOCAL;
+}
+
+#ifdef CONFIG_CONSOLE_POLL
+
+static int tegra_utc_poll_init(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_hw_init(tup);
+	return 0;
+}
+
+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();
+
+	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();
+
+	tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
+}
+
+#endif
+
+static const struct uart_ops tegra_utc_uart_ops = {
+	.tx_empty = tegra_utc_tx_empty,
+	.set_mctrl = tegra_utc_set_mctrl,
+	.get_mctrl = tegra_utc_get_mctrl,
+	.stop_tx = tegra_utc_stop_tx,
+	.start_tx = tegra_utc_start_tx,
+	.stop_rx = tegra_utc_stop_rx,
+	.startup = tegra_utc_startup,
+	.shutdown = tegra_utc_shutdown,
+	.set_termios = tegra_utc_set_termios,
+#ifdef CONFIG_CONSOLE_POLL
+	.poll_init = tegra_utc_poll_init,
+	.poll_get_char = tegra_utc_get_poll_char,
+	.poll_put_char = tegra_utc_put_poll_char,
+#endif
+};
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+#define TEGRA_UTC_DEFAULT_FIFO_THRESHOLD	0x4
+#define TEGRA_UTC_EARLYCON_MAX_BURST_SIZE	128
+
+static void tegra_utc_putc(struct uart_port *port, unsigned char c)
+{
+	writel(c, port->membase + TEGRA_UTC_DATA);
+}
+
+static void tegra_utc_early_write(struct console *con, const char *s, unsigned int n)
+{
+	struct earlycon_device *dev = con->data;
+
+	while (n) {
+		u32 burst_size = TEGRA_UTC_EARLYCON_MAX_BURST_SIZE;
+
+		burst_size -= readl(dev->port.membase + TEGRA_UTC_FIFO_OCCUPANCY);
+		if (n < burst_size)
+			burst_size = n;
+
+		uart_console_write(&dev->port, s, burst_size, tegra_utc_putc);
+
+		n -= burst_size;
+		s += burst_size;
+	}
+}
+
+static int __init tegra_utc_early_console_setup(struct earlycon_device *device, const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	/* Configure TX */
+	writel(TEGRA_UTC_COMMAND_FLUSH | TEGRA_UTC_COMMAND_RESET,
+		device->port.membase + TEGRA_UTC_COMMAND);
+	writel(TEGRA_UTC_DEFAULT_FIFO_THRESHOLD, device->port.membase + TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear and mask all the interrupts. */
+	writel(TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
+		device->port.membase + TEGRA_UTC_INTR_CLEAR);
+
+	writel(0x0, device->port.membase + TEGRA_UTC_INTR_MASK);
+	writel(0x0, device->port.membase + TEGRA_UTC_INTR_SET);
+
+	/* Enable TX. */
+	writel(TEGRA_UTC_ENABLE_CLIENT_ENABLE, device->port.membase + TEGRA_UTC_ENABLE);
+
+	device->con->write = tegra_utc_early_write;
+
+	return 0;
+}
+OF_EARLYCON_DECLARE(tegra_utc, "nvidia,tegra264-utc", tegra_utc_early_console_setup);
+
+static void tegra_utc_console_putchar(struct uart_port *port, unsigned char ch)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
+}
+
+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;
+	tup->console.data	= &tegra_utc_driver;
+#endif
+
+	uart_read_port_properties(&tup->port);
+}
+
+static int tegra_utc_register_port(struct tegra_utc_port *tup)
+{
+	int ret;
+
+	ret = uart_add_one_port(&tegra_utc_driver, &tup->port);
+	if (ret)
+		return ret;
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+	register_console(&tup->console);
+#endif
+
+	return 0;
+}
+
+static int tegra_utc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct tegra_utc_port *tup;
+	int ret;
+
+	tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port), GFP_KERNEL);
+	if (!tup)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "tx-threshold", &tup->tx_threshold);
+	if (ret)
+		return dev_err_probe(dev, ret, "missing tx-threshold device-tree property\n");
+
+	ret = of_property_read_u32(np, "rx-threshold", &tup->rx_threshold);
+	if (ret)
+		return dev_err_probe(dev, ret, "missing rx-threshold device-tree property\n");
+
+	tup->irq = platform_get_irq(pdev, 0);
+	if (tup->irq < 0)
+		return tup->irq;
+
+	tup->soc = of_device_get_match_data(&pdev->dev);
+
+	tup->tx_base = devm_platform_ioremap_resource_byname(pdev, "tx");
+	if (IS_ERR(tup->tx_base))
+		return PTR_ERR(tup->tx_base);
+
+	tup->rx_base = devm_platform_ioremap_resource_byname(pdev, "rx");
+	if (IS_ERR(tup->rx_base))
+		return PTR_ERR(tup->rx_base);
+
+	tegra_utc_setup_port(&pdev->dev, tup);
+	platform_set_drvdata(pdev, tup);
+
+	return tegra_utc_register_port(tup);
+}
+
+static void tegra_utc_remove(struct platform_device *pdev)
+{
+	struct tegra_utc_port *tup = platform_get_drvdata(pdev);
+
+	uart_remove_one_port(&tegra_utc_driver, &tup->port);
+}
+
+static const struct tegra_utc_soc tegra264_utc_soc = {
+	.fifosize = 128,
+};
+
+static const struct of_device_id tegra_utc_of_match[] = {
+	{ .compatible = "nvidia,tegra264-utc", .data = &tegra264_utc_soc },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tegra_utc_of_match);
+
+static struct platform_driver tegra_utc_platform_driver = {
+	.probe = tegra_utc_probe,
+	.remove = tegra_utc_remove,
+	.driver = {
+		.name = "tegra-utc",
+		.of_match_table = tegra_utc_of_match,
+	},
+};
+
+static int __init tegra_utc_init(void)
+{
+	int ret;
+
+	ret = uart_register_driver(&tegra_utc_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&tegra_utc_platform_driver);
+	if (ret) {
+		uart_unregister_driver(&tegra_utc_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(tegra_utc_init);
+
+static void __exit tegra_utc_exit(void)
+{
+	platform_driver_unregister(&tegra_utc_platform_driver);
+	uart_unregister_driver(&tegra_utc_driver);
+}
+module_exit(tegra_utc_exit);
+
+MODULE_AUTHOR("Kartik Rajput <kkartik@nvidia.com>");
+MODULE_DESCRIPTION("Tegra UART Trace Controller");
+MODULE_LICENSE("GPL");
-- 
2.43.0

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@nvidia.com>

> --- /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

Thanks for reviewing the patch Jiri!

On Tue, 2025-02-11 at 08:23 +0100, Jiri Slaby wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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@nvidia.com>
> 
> > --- /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?

Not needed, I will remove this.

> 
> > +#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?
> 

Not needed, I will remove this.

> > +#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.

I will change the ordering from LSB to MSB.

> 
> > +#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?

The idea to add this struct was to allow for a simpler future expansion
of the SoC data. But I agree, at this time, this struct can be removed.

I will update the patch and move fifosize to `struct tegra_utc_port`
instead.

> 
> > +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?
> 

Ack, this is redundant, uart_port::irq can be used instead of this. I
will remove this.

> > +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()?
> 

Ack, I will update the patch to use uart_port_tx() instead.

> > +{
> > +     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.
> 

Ack.

> > +     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".

Ack.

> 
> > +             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'.
> 

Ack.

> > +     }
> > +
> > +     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).
> 

Makes sense, I will update this logic.

> > +}
> 
> > +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?

Correct, this cannot be shared.

> 
> > +     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?
> 

Writes are allowed for RX client, only the writes to the RX client
data register are not allowed.

> > +     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()?

Ack.

> 
> > +     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.

Ack.

> 
> > +     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.
> 

Ack.

> > +     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()?
> 

Ack, I will add this in the next patch.

> > +     uart_remove_one_port(&tegra_utc_driver, &tup->port);
> > +}
> 
> thanks,
> --
> js
> suse labs

Thanks,
Kartik

On Tue, Feb 11, 2025 at 08:23:47AM +0100, Jiri Slaby wrote:
> On 11. 02. 25, 7:19, Kartik Rajput wrote:

...

> > +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).
> 
> > +}

I am also wondering why _irqsave / _irqrestore? Don't you have interrupts
already being disabled at this point?

-- 
With Best Regards,
Andy Shevchenko

On Tue, Feb 11, 2025 at 11:49:45AM +0530, 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.

...

+ bits.h

> +#include <linux/console.h>

+ container_of.h
+ device.h
+ err.h
+ io.h

> +#include <linux/kthread.h>

+ kfifo.h

> +#include <linux/module.h>

+ mod_devicetable.h

> +#include <linux/of.h>

> +#include <linux/of_device.h>

Use property.h (see also below).

> +#include <linux/platform_device.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>

+ types.h

...

> +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;
> +	u32 status;
> +	int sysrq;
> +	u32 ch;
> +
> +	while (--max_chars) {

Wouldn't while (max_chars--) { suffice?

> +		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);
> +		uart_port_lock(&tup->port);
> +
> +		if (!sysrq)
> +			tty_insert_flip_char(port, ch, flag);
> +	}
> +
> +	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);

As said previously, why _irqsave?

> +	/* 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;
> +}

...

> +{
> +	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);
> +	if (ret < 0) {
> +		dev_err(port->dev, "failed to register interrupt handler\n");

Instead of these LoCs...

> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

> +}

...

> +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();

No way out? HW might get stuck...

> +	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();

Ditto.

> +	tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
> +}

...

> +static struct uart_driver tegra_utc_driver = {
> +	.driver_name	= "tegra-utc",
> +	.dev_name	= "ttyUTC",

> +	.nr		= UART_NR

Leave trailing comma.

> +};

...

> +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;
> +	tup->console.data	= &tegra_utc_driver;
> +#endif
> +
> +	uart_read_port_properties(&tup->port);

No failure handling? In some cases it might return an error.

> +}

...

> +static int tegra_utc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct tegra_utc_port *tup;
> +	int ret;
> +
> +	tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port), GFP_KERNEL);

sizeof(*tup)

> +	if (!tup)
> +		return -ENOMEM;

> +	ret = of_property_read_u32(np, "tx-threshold", &tup->tx_threshold);

device_property_read_u32()

> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing tx-threshold device-tree property\n");
> +
> +	ret = of_property_read_u32(np, "rx-threshold", &tup->rx_threshold);

Ditto.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing rx-threshold device-tree property\n");

> +	tup->irq = platform_get_irq(pdev, 0);
> +	if (tup->irq < 0)
> +		return tup->irq;

uart_read_port_properties() does this for you.

> +	tup->soc = of_device_get_match_data(&pdev->dev);

device_get_match_data()

> +	tup->tx_base = devm_platform_ioremap_resource_byname(pdev, "tx");
> +	if (IS_ERR(tup->tx_base))
> +		return PTR_ERR(tup->tx_base);
> +
> +	tup->rx_base = devm_platform_ioremap_resource_byname(pdev, "rx");
> +	if (IS_ERR(tup->rx_base))
> +		return PTR_ERR(tup->rx_base);
> +
> +	tegra_utc_setup_port(&pdev->dev, tup);
> +	platform_set_drvdata(pdev, tup);
> +
> +	return tegra_utc_register_port(tup);
> +}

-- 
With Best Regards,
Andy Shevchenko

Thanks for reviewing the patch Andy!

On Tue, 2025-02-11 at 12:01 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Feb 11, 2025 at 11:49:45AM +0530, 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.
> 
> ...
> 
> + bits.h
> 
> > +#include <linux/console.h>
> 
> + container_of.h
> + device.h
> + err.h
> + io.h
> 
> > +#include <linux/kthread.h>
> 
> + kfifo.h
> 
> > +#include <linux/module.h>
> 
> + mod_devicetable.h
> 
> > +#include <linux/of.h>
> 
> > +#include <linux/of_device.h>
> 
> Use property.h (see also below).
> 
> > +#include <linux/platform_device.h>
> > +#include <linux/serial.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> 
> + types.h
> 

Ack.

> ...
> 
> > +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;
> > +     u32 status;
> > +     int sysrq;
> > +     u32 ch;
> > +
> > +     while (--max_chars) {
> 
> Wouldn't while (max_chars--) { suffice?
> 

Yes, I should be using `max_chars--` instead. I will update this.

> > +             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);
> > +             uart_port_lock(&tup->port);
> > +
> > +             if (!sysrq)
> > +                     tty_insert_flip_char(port, ch, flag);
> > +     }
> > +
> > +     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);
> 
> As said previously, why _irqsave?
> 

You're right, at this point the IRQs are already disabled so this is
not really required. I will use `uart_port_lock/unlock()` instead.

> > +     /* 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;
> > +}
> 
> ...
> 
> > +{
> > +     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);
> > +     if (ret < 0) {
> > +             dev_err(port->dev, "failed to register interrupt
> > handler\n");
> 
> Instead of these LoCs...
> 
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> 
>         return ret;

Ack.

> 
> > +}
> 
> ...
> 
> > +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();
> 
> No way out? HW might get stuck...

Ack. I will update this to 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();
> 
> Ditto.

Ack.

> 
> > +     tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
> > +}
> 
> ...
> 
> > +static struct uart_driver tegra_utc_driver = {
> > +     .driver_name    = "tegra-utc",
> > +     .dev_name       = "ttyUTC",
> 
> > +     .nr             = UART_NR
> 
> Leave trailing comma.

Ack.

> 
> > +};
> 
> ...
> 
> > +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;
> > +     tup->console.data       = &tegra_utc_driver;
> > +#endif
> > +
> > +     uart_read_port_properties(&tup->port);
> 
> No failure handling? In some cases it might return an error.

Ack, I will propagate the error properly.

> 
> > +}
> 
> ...
> 
> > +static int tegra_utc_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct device *dev = &pdev->dev;
> > +     struct tegra_utc_port *tup;
> > +     int ret;
> > +
> > +     tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port),
> > GFP_KERNEL);
> 
> sizeof(*tup)
> 
> > +     if (!tup)
> > +             return -ENOMEM;
> 
> > +     ret = of_property_read_u32(np, "tx-threshold", &tup-
> > >tx_threshold);
> 
> device_property_read_u32()

Ack.

> 
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing tx-threshold
> > device-tree property\n");
> > +
> > +     ret = of_property_read_u32(np, "rx-threshold", &tup-
> > >rx_threshold);
> 
> Ditto.

Ack.

> 
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing rx-threshold
> > device-tree property\n");
> 
> > +     tup->irq = platform_get_irq(pdev, 0);
> > +     if (tup->irq < 0)
> > +             return tup->irq;
> 
> uart_read_port_properties() does this for you.

Ack.

> 
> > +     tup->soc = of_device_get_match_data(&pdev->dev);
> 
> device_get_match_data()

Ack.

> 
> > +     tup->tx_base = devm_platform_ioremap_resource_byname(pdev,
> > "tx");
> > +     if (IS_ERR(tup->tx_base))
> > +             return PTR_ERR(tup->tx_base);
> > +
> > +     tup->rx_base = devm_platform_ioremap_resource_byname(pdev,
> > "rx");
> > +     if (IS_ERR(tup->rx_base))
> > +             return PTR_ERR(tup->rx_base);
> > +
> > +     tegra_utc_setup_port(&pdev->dev, tup);
> > +     platform_set_drvdata(pdev, tup);
> > +
> > +     return tegra_utc_register_port(tup);
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks,
Kartik

On Tue, Feb 11, 2025 at 12:01:11PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 11, 2025 at 11:49:45AM +0530, Kartik Rajput wrote:

...

> > +#include <linux/of.h>
> 
> > +#include <linux/of_device.h>
> 
> Use property.h (see also below).

To be more precise. The above proposal is to _replace_ linux/of*.h with
linux/property.h and use APIs from there.

-- 
With Best Regards,
Andy Shevchenko