[PATCH v3 0/2] Add support for Tegra UART Trace Controller (UTC) client

[PATCH v3 0/2] Add support for Tegra UART Trace Controller (UTC) client

[Kartik Rajput]

The Tegra UTC (UART Trace Controller) is a hardware controller that allows
multiple systems within the Tegra SoC to share a hardware UART interface.
It supports up to 16 clients, with each client having its own interrupt and a
FIFO buffer for both RX (receive) and TX (transmit), each capable of holding
128 characters.

The Tegra UTC uses 8-N-1 configuration and operates on a pre-configured
baudrate, which is configured by the bootloader.

Kartik Rajput (2):
  dt-bindings: serial: Add bindings for nvidia,tegra264-utc
  serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

 .../bindings/serial/nvidia,tegra264-utc.yaml  |  73 ++
 drivers/tty/serial/Kconfig                    |  23 +
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/tegra-utc.c                | 634 ++++++++++++++++++
 4 files changed, 731 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
 create mode 100644 drivers/tty/serial/tegra-utc.c

-- 
2.43.0

[PATCH v3 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Kartik Rajput]

The Tegra UTC (UART Trace Controller) allows multiple clients within
the Tegra SoC to share a physical UART interface. It supports up to 16
clients. Each client operates as an independent UART endpoint with a
dedicated interrupt and 128-character TX/RX FIFOs.

Add device tree binding documentation for the Tegra UTC client.

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
v1 -> v2:
	* Removed current-speed as it might not be accurate as the Tegra
	  UTC multiplex data from various clients.
	* Use 'tx-threshold' and 'rx-threshold' properties defined in
	  serial.yaml instead of 'nvidia,utc-fifo-threshold'.
	* Add serial.yaml reference.
	* Define minimum and maximum values for threshold.
	* Rephrase the documentation to clarify that we are documenting
	  the Tegra UTC clients and not the controller itself.
---
 .../bindings/serial/nvidia,tegra264-utc.yaml  | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml

diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml b/Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
new file mode 100644
index 000000000000..572cc574da64
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/nvidia,tegra264-utc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVIDIA Tegra UTC (UART Trace Controller) client
+
+maintainers:
+  - Kartik Rajput <kkartik@nvidia.com>
+  - Thierry Reding <thierry.reding@gmail.com>
+  - Jonathan Hunter <jonathanh@nvidia.com>
+
+description:
+  Represents a client interface of the Tegra UTC (UART Trace Controller). The
+  Tegra UTC allows multiple clients within the Tegra SoC to share a physical
+  UART interface. It supports up to 16 clients. Each client operates as an
+  independent UART endpoint with a dedicated interrupt and 128-character TX/RX
+  FIFOs.
+
+  The Tegra UTC clients use 8-N-1 configuration and operates on a baudrate
+  configured by the bootloader at the controller level.
+
+allOf:
+  - $ref: serial.yaml#
+
+properties:
+  compatible:
+    const: nvidia,tegra264-utc
+
+  reg:
+    items:
+      - description: TX region.
+      - description: RX region.
+
+  reg-names:
+    items:
+      - const: tx
+      - const: rx
+
+  interrupts:
+    maxItems: 1
+
+  tx-threshold:
+    minimum: 1
+    maximum: 128
+
+  rx-threshold:
+    minimum: 1
+    maximum: 128
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - tx-threshold
+  - rx-threshold
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    tegra_utc: serial@c4e0000 {
+        compatible = "nvidia,tegra264-utc";
+        reg = <0xc4e0000 0x8000>, <0xc4e8000 0x8000>;
+        reg-names = "tx", "rx";
+        interrupts = <GIC_SPI 514 IRQ_TYPE_LEVEL_HIGH>;
+        tx-threshold = <4>;
+        rx-threshold = <4>;
+    };
-- 
2.43.0

[PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

The Tegra264 SoC supports the UART Trace Controller (UTC), 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>
---
v2 -> v3:
	* Update included header files.
	* Remove `struct tegra_utc_soc` and move fifosize to `struct
	  tegra_utc_port`.
	* Remove `irq` from `struct tegra_utc_port` and use
	  uart_port::irq instead.
	* Update `tegra_utc_tx_chars` to use `uart_port_tx` API
	  instead.
	* Remove `flag` variable from `tegra_utc_rx_chars()` as it was
	  not really required.
	* Removed mask for `ch` in `tegra_utc_rx_chars()` as both
	  `uart_handle_sysrq_char()` and `tty_insert_flip_char()` takes
	  u8 value.
	* Update `tegra_utc_isr()` to return IRQ_HANDLED only when it
	  actually handles the interrupt.
	* Use uart_port_lock/unlock APIs in `tegra_utc_isr()` instead of
	  using irqsave/irqrestore counterparts.
	* Update `tegra_utc_get_poll_char()` logic to return NO_POLL_CHAR
	  if FIFO is empty.
	* Use `read_poll_timeout_atomic()` in `tegra_utc_put_poll_char()`
	  to avoid stall.
	* Introduce write_atomic/thread console APIs to make the driver
	  CON_NBCON compatible.
	* Unregister the console in `tegra_utc_remove()`.
	* Use `max_chars--` instead of `--max_chars` in
	  `tegra_utc_rx_chars()`.
	* Improve error handling logic of `tegra_utc_startup()` if
	  `request_irq()` fails.
	* Add trailing comma in tegra_utc_driver.
	* Propagate error if `uart_read_port_properties()` fails.
	* replace of_* APIs with device_* APIs.
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 | 634 +++++++++++++++++++++++++++++++++
 3 files changed, 658 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..7dfc242b25c4
--- /dev/null
+++ b/drivers/tty/serial/tegra-utc.c
@@ -0,0 +1,634 @@
+// 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/bits.h>
+#include <linux/console.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kfifo.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/platform_device.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/types.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_RESET			BIT(0)
+#define TEGRA_UTC_COMMAND_FLUSH			BIT(1)
+
+#define TEGRA_UTC_DATA				0x20
+
+#define TEGRA_UTC_FIFO_STATUS			0x100
+#define TEGRA_UTC_FIFO_EMPTY			BIT(0)
+#define TEGRA_UTC_FIFO_FULL			BIT(1)
+#define TEGRA_UTC_FIFO_REQ			BIT(2)
+#define TEGRA_UTC_FIFO_OVERFLOW			BIT(3)
+#define TEGRA_UTC_FIFO_TIMEOUT			BIT(4)
+
+#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_EMPTY			BIT(0)
+#define TEGRA_UTC_INTR_FULL			BIT(1)
+#define TEGRA_UTC_INTR_REQ			BIT(2)
+#define TEGRA_UTC_INTR_OVERFLOW			BIT(3)
+#define TEGRA_UTC_INTR_TIMEOUT			BIT(4)
+
+#define UART_NR					16
+
+struct tegra_utc_port {
+#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;
+
+	unsigned int fifosize;
+	u32 tx_threshold;
+	u32 rx_threshold;
+};
+
+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_chars(struct tegra_utc_port *tup)
+{
+	struct uart_port *port = &tup->port;
+	unsigned int pending;
+	u8 c;
+
+	pending = uart_port_tx(port, c,
+		     !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL),
+		     tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));
+
+	if (pending)
+		return true;
+
+	return false;
+}
+
+static void tegra_utc_rx_chars(struct tegra_utc_port *tup)
+{
+	struct tty_port *port = &tup->port.state->port;
+	unsigned int max_chars = 256;
+	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);
+		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);
+		uart_port_lock(&tup->port);
+
+		if (!sysrq)
+			tty_insert_flip_char(port, ch, TTY_NORMAL);
+	}
+
+	tty_flip_buffer_push(port);
+}
+
+static irqreturn_t tegra_utc_isr(int irq, void *dev_id)
+{
+	struct tegra_utc_port *tup = dev_id;
+	unsigned int handled = 0;
+	u32 status;
+
+	uart_port_lock(&tup->port);
+
+	/* 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);
+			handled = 1;
+		}
+	} 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);
+			handled = 1;
+		}
+	} while (status);
+
+	uart_port_unlock(&tup->port);
+
+	return IRQ_RETVAL(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(port->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;
+}
+
+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(port->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);
+	u32 val;
+
+	if (tegra_utc_rx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_EMPTY)
+		return NO_POLL_CHAR;
+
+	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);
+	u32 val;
+
+	read_poll_timeout_atomic(tegra_utc_tx_readl, val, !(val & TEGRA_UTC_FIFO_FULL),
+				 0, USEC_PER_SEC, false, tup, TEGRA_UTC_FIFO_STATUS);
+
+	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_atomic(struct console *cons, struct nbcon_write_context *wctxt)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+	unsigned int len;
+	char *outbuf;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	outbuf = wctxt->outbuf;
+	len = wctxt->len;
+
+	while (len) {
+		u32 burst_size = tup->fifosize;
+
+		burst_size -= tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_OCCUPANCY);
+		if (len < burst_size)
+			burst_size = len;
+
+		uart_console_write(&tup->port, outbuf, burst_size, tegra_utc_console_putchar);
+
+		outbuf += burst_size;
+		len -= burst_size;
+	};
+
+	nbcon_exit_unsafe(wctxt);
+}
+
+static void tegra_utc_console_write_thread(struct console *cons, struct nbcon_write_context *wctxt)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+	unsigned int len = READ_ONCE(wctxt->len);
+	unsigned int i;
+	u32 val;
+
+	for (i = 0; i < len; i++) {
+		if (!nbcon_enter_unsafe(wctxt))
+			break;
+
+		read_poll_timeout_atomic(tegra_utc_tx_readl, val, !(val & TEGRA_UTC_FIFO_FULL),
+					 0, USEC_PER_SEC, false, tup, TEGRA_UTC_FIFO_STATUS);
+		uart_console_write(&tup->port, wctxt->outbuf + i, 1, tegra_utc_console_putchar);
+
+		if (!nbcon_exit_unsafe(wctxt))
+			break;
+	}
+
+}
+
+static void tegra_utc_console_device_lock(struct console *cons, unsigned long *flags)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+	struct uart_port *port = &tup->port;
+
+	__uart_port_lock_irqsave(port, flags);
+}
+
+static void tegra_utc_console_device_unlock(struct console *cons, unsigned long flags)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+	struct uart_port *port = &tup->port;
+
+	__uart_port_unlock_irqrestore(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 int tegra_utc_setup_port(struct device *dev, struct tegra_utc_port *tup)
+{
+	tup->port.dev			= dev;
+	tup->port.fifosize		= tup->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_atomic	= tegra_utc_console_write_atomic;
+	tup->console.write_thread	= tegra_utc_console_write_thread;
+	tup->console.device_lock	= tegra_utc_console_device_lock;
+	tup->console.device_unlock	= tegra_utc_console_device_unlock;
+	tup->console.device		= uart_console_device;
+	tup->console.setup		= tegra_utc_console_setup;
+	tup->console.flags		= CON_PRINTBUFFER | CON_NBCON;
+	tup->console.data		= &tegra_utc_driver;
+#endif
+
+	return 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)
+{
+	const unsigned int *soc_fifosize;
+	struct device *dev = &pdev->dev;
+	struct tegra_utc_port *tup;
+	int ret;
+
+	tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
+	if (!tup)
+		return -ENOMEM;
+
+	ret = device_property_read_u32(dev, "tx-threshold", &tup->tx_threshold);
+	if (ret)
+		return dev_err_probe(dev, ret, "missing tx-threshold device-tree property\n");
+
+	ret = device_property_read_u32(dev, "rx-threshold", &tup->rx_threshold);
+	if (ret)
+		return dev_err_probe(dev, ret, "missing rx-threshold device-tree property\n");
+
+	soc_fifosize = device_get_match_data(&pdev->dev);
+	tup->fifosize = *soc_fifosize;
+
+	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);
+
+	ret = tegra_utc_setup_port(&pdev->dev, tup);
+	if (ret)
+		dev_err_probe(dev, ret, "failed to setup uart port\n");
+
+	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);
+
+	unregister_console(&tup->console);
+	uart_remove_one_port(&tegra_utc_driver, &tup->port);
+}
+
+static const unsigned int tegra264_utc_soc = 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

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:
> The Tegra264 SoC supports the UART Trace Controller (UTC), 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.

...

> +/*
> + * NVIDIA Tegra UTC (UART Trace Controller) driver.
> + */

Can be a single line.

...

> +#include <linux/bits.h>
> +#include <linux/console.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>

> +#include <linux/io.h>
> +#include <linux/iopoll.h>

iopoll.h guarantees to include io.h in case you want to have less lines here.
(yeah, I know that the header guarantees is a tribal knowledge, it's undocumented)

> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

> +#include <linux/of.h>

Is this being used now?

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

...

> +#define UART_NR					16

Bad naming, calling for collisions. Move it to the driver's namespace.

...

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

Here...

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

...and here the potential of deduplication by introducing an additional constant:

#define TEGRA_UTC_INTR_COMMON	\
	(...)

(choose better name)

> +	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_chars(struct tegra_utc_port *tup)
> +{
> +	struct uart_port *port = &tup->port;
> +	unsigned int pending;
> +	u8 c;
> +
> +	pending = uart_port_tx(port, c,
> +		     !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL),
> +		     tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));

Make the last two to reside in temporary variables with self-explanatory names.

> +

Redundant blank line.

> +	if (pending)
> +		return true;
> +
> +	return false;

	return pending;

> +}

...

> +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(port->irq, tegra_utc_isr, 0, dev_name(port->dev), tup);

Seems the same Q stands about sharing, perhaps a comment why it's expected to
be always exclusive?

> +	if (ret < 0)
> +		dev_err(port->dev, "failed to register interrupt handler\n");
> +
> +	return ret;
> +}

...

> +	for (i = 0; i < len; i++) {
> +		if (!nbcon_enter_unsafe(wctxt))
> +			break;
> +
> +		read_poll_timeout_atomic(tegra_utc_tx_readl, val, !(val & TEGRA_UTC_FIFO_FULL),
> +					 0, USEC_PER_SEC, false, tup, TEGRA_UTC_FIFO_STATUS);

No error check?

> +		uart_console_write(&tup->port, wctxt->outbuf + i, 1, tegra_utc_console_putchar);
> +
> +		if (!nbcon_exit_unsafe(wctxt))
> +			break;
> +	}

> +

Unneeded blank line.

> +}

...

> +static int tegra_utc_probe(struct platform_device *pdev)
> +{
> +	const unsigned int *soc_fifosize;
> +	struct device *dev = &pdev->dev;
> +	struct tegra_utc_port *tup;
> +	int ret;
> +
> +	tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
> +	if (!tup)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32(dev, "tx-threshold", &tup->tx_threshold);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "missing tx-threshold device-tree property\n");

' device-tree' is redundant part.

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

Ditto.

Also in a form of

		return dev_err_probe(dev, ret, "missing %s property\n", "rx-threshold");

in both cases the size of the object file will be smaller by a couple of dozens
of bytes.

> +	soc_fifosize = device_get_match_data(&pdev->dev);
> +	tup->fifosize = *soc_fifosize;
> +
> +	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);
> +
> +	ret = tegra_utc_setup_port(&pdev->dev, tup);
> +	if (ret)
> +		dev_err_probe(dev, ret, "failed to setup uart port\n");
> +
> +	platform_set_drvdata(pdev, tup);
> +
> +	return tegra_utc_register_port(tup);
> +}

...

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

Just

	return ret;

will be good instead of the above 4 LoCs.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Jiri Slaby]

On 12. 02. 25, 16:09, Andy Shevchenko wrote:
>> +static bool tegra_utc_tx_chars(struct tegra_utc_port *tup)
>> +{
>> +	struct uart_port *port = &tup->port;
>> +	unsigned int pending;
>> +	u8 c;
>> +
>> +	pending = uart_port_tx(port, c,
>> +		     !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL),
>> +		     tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));
> 
> Make the last two to reside in temporary variables with self-explanatory names.

Not sure what you mean here? They are needed to be evaluated 
(read/written) in every loop.

-- 
js
suse labs

Re: [PATCH v3 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Krzysztof Kozlowski]

On 12/02/2025 11:41, Kartik Rajput wrote:
> The Tegra UTC (UART Trace Controller) allows multiple clients within
> the Tegra SoC to share a physical UART interface. It supports up to 16
> clients. Each client operates as an independent UART endpoint with a
> dedicated interrupt and 128-character TX/RX FIFOs.
> 
> Add device tree binding documentation for the Tegra UTC client.
> 
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Andy Shevchenko]

On Thu, Feb 13, 2025 at 08:38:15AM +0100, Jiri Slaby wrote:
> On 12. 02. 25, 16:09, Andy Shevchenko wrote:

...

> > > +	pending = uart_port_tx(port, c,
> > > +		     !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL),
> > > +		     tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));
> > 
> > Make the last two to reside in temporary variables with self-explanatory names.
> 
> Not sure what you mean here? They are needed to be evaluated (read/written)
> in every loop.

Ah, uart_port_tx() is a macro!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

Hi Andy,

Thanks for the review!


On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:
> > The Tegra264 SoC supports the UART Trace Controller (UTC), 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.
> 
> ...
> 
> > +/*
> > + * NVIDIA Tegra UTC (UART Trace Controller) driver.
> > + */
> 
> Can be a single line.
> 

Ack.

> ...
> 
> > +#include <linux/bits.h>
> > +#include <linux/console.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> 
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> 
> iopoll.h guarantees to include io.h in case you want to have less
> lines here.
> (yeah, I know that the header guarantees is a tribal knowledge, it's
> undocumented)

Ack. I will remove io.h from here.

> 
> > +#include <linux/kfifo.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> 
> > +#include <linux/of.h>
> 
> Is this being used now?
> 

No, I will remove this.

> > +#include <linux/property.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/serial.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/types.h>
> 
> ...
> 
> > +#define UART_NR                                      16
> 
> Bad naming, calling for collisions. Move it to the driver's
> namespace.
> 

Ack.

> ...
> 
> > +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);
> 
> Here...
> 
> > +     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);
> 
> ...and here the potential of deduplication by introducing an
> additional constant:
> 
> #define TEGRA_UTC_INTR_COMMON   \
>         (...)
> 
> (choose better name)
> 

Ack.

I think TEGRA_UTC_INTR_COMMON would be a good name for this macro since
these interrupts are common between TX and RX client.

> > +     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_chars(struct tegra_utc_port *tup)
> > +{
> > +     struct uart_port *port = &tup->port;
> > +     unsigned int pending;
> > +     u8 c;
> > +
> > +     pending = uart_port_tx(port, c,
> > +                  !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS)
> > & TEGRA_UTC_FIFO_FULL),
> > +                  tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));
> 
> Make the last two to reside in temporary variables with self-
> explanatory names.
> 
> > +
> 
> Redundant blank line.

Ack.

> 
> > +     if (pending)
> > +             return true;
> > +
> > +     return false;
> 
>         return pending;
> 

Ack.

> > +}
> 
> ...
> 
> > +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(port->irq, tegra_utc_isr, 0, dev_name(port-
> > >dev), tup);
> 
> Seems the same Q stands about sharing, perhaps a comment why it's
> expected to
> be always exclusive?

Ack. I will drop a comment here.

> 
> > +     if (ret < 0)
> > +             dev_err(port->dev, "failed to register interrupt
> > handler\n");
> > +
> > +     return ret;
> > +}
> 
> ...
> 
> > +     for (i = 0; i < len; i++) {
> > +             if (!nbcon_enter_unsafe(wctxt))
> > +                     break;
> > +
> > +             read_poll_timeout_atomic(tegra_utc_tx_readl, val,
> > !(val & TEGRA_UTC_FIFO_FULL),
> > +                                      0, USEC_PER_SEC, false, tup,
> > TEGRA_UTC_FIFO_STATUS);
> 
> No error check?
> 

I'm not sure about this. The case where the TX FIFO doesn't clear up,
even after polling for 1 second, is highly unlikely, especially since
there's no flow control involved here. Even if that did happen, writing
to the TX FIFO should just result in an overflow, which is probably
acceptable in this scenario.

> > +             uart_console_write(&tup->port, wctxt->outbuf + i, 1,
> > tegra_utc_console_putchar);
> > +
> > +             if (!nbcon_exit_unsafe(wctxt))
> > +                     break;
> > +     }
> 
> > +
> 
> Unneeded blank line.

Ack.

> 
> > +}
> 
> ...
> 
> > +static int tegra_utc_probe(struct platform_device *pdev)
> > +{
> > +     const unsigned int *soc_fifosize;
> > +     struct device *dev = &pdev->dev;
> > +     struct tegra_utc_port *tup;
> > +     int ret;
> > +
> > +     tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
> > +     if (!tup)
> > +             return -ENOMEM;
> > +
> > +     ret = device_property_read_u32(dev, "tx-threshold", &tup-
> > >tx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing tx-threshold
> > device-tree property\n");
> 
> ' device-tree' is redundant part.

Ack.

> 
> > +     ret = device_property_read_u32(dev, "rx-threshold", &tup-
> > >rx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing rx-threshold
> > device-tree property\n");
> 
> Ditto.
> 
> Also in a form of
> 
>                 return dev_err_probe(dev, ret, "missing %s
> property\n", "rx-threshold");
> 
> in both cases the size of the object file will be smaller by a couple
> of dozens
> of bytes.

Ack.

> 
> > +     soc_fifosize = device_get_match_data(&pdev->dev);
> > +     tup->fifosize = *soc_fifosize;
> > +
> > +     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);
> > +
> > +     ret = tegra_utc_setup_port(&pdev->dev, tup);
> > +     if (ret)
> > +             dev_err_probe(dev, ret, "failed to setup uart
> > port\n");
> > +
> > +     platform_set_drvdata(pdev, tup);
> > +
> > +     return tegra_utc_register_port(tup);
> > +}
> 
> ...
> 
> > +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;
> 
> Just
> 
>         return ret;
> 
> will be good instead of the above 4 LoCs.
> 

Ack.

> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[andriy.shevchenko]

On Thu, Feb 13, 2025 at 09:05:36AM +0000, Kartik Rajput wrote:
> On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:

...

> > > +     for (i = 0; i < len; i++) {
> > > +             if (!nbcon_enter_unsafe(wctxt))
> > > +                     break;
> > > +
> > > +             read_poll_timeout_atomic(tegra_utc_tx_readl, val,
> > > !(val & TEGRA_UTC_FIFO_FULL),
> > > +                                      0, USEC_PER_SEC, false, tup,
> > > TEGRA_UTC_FIFO_STATUS);
> > 
> > No error check?
> 
> I'm not sure about this. The case where the TX FIFO doesn't clear up,
> even after polling for 1 second, is highly unlikely, especially since
> there's no flow control involved here. Even if that did happen, writing
> to the TX FIFO should just result in an overflow, which is probably
> acceptable in this scenario.

Perhaps a warning (debug?) message in such a case?

> > > +             uart_console_write(&tup->port, wctxt->outbuf + i, 1,
> > > tegra_utc_console_putchar);
> > > +
> > > +             if (!nbcon_exit_unsafe(wctxt))
> > > +                     break;
> > > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

On Thu, 2025-02-13 at 11:55 +0200, andriy.shevchenko@linux.intel.com
wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Feb 13, 2025 at 09:05:36AM +0000, Kartik Rajput wrote:
> > On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:
> 
> ...
> 
> > > > +     for (i = 0; i < len; i++) {
> > > > +             if (!nbcon_enter_unsafe(wctxt))
> > > > +                     break;
> > > > +
> > > > +             read_poll_timeout_atomic(tegra_utc_tx_readl, val,
> > > > !(val & TEGRA_UTC_FIFO_FULL),
> > > > +                                      0, USEC_PER_SEC, false,
> > > > tup,
> > > > TEGRA_UTC_FIFO_STATUS);
> > > 
> > > No error check?
> > 
> > I'm not sure about this. The case where the TX FIFO doesn't clear
> > up,
> > even after polling for 1 second, is highly unlikely, especially
> > since
> > there's no flow control involved here. Even if that did happen,
> > writing
> > to the TX FIFO should just result in an overflow, which is probably
> > acceptable in this scenario.
> 
> Perhaps a warning (debug?) message in such a case?

I would prefer avoiding any prints in this function, as we are writing
debug messages to the UART HW here.

> 
> > > > +             uart_console_write(&tup->port, wctxt->outbuf + i,
> > > > 1,
> > > > tegra_utc_console_putchar);
> > > > +
> > > > +             if (!nbcon_exit_unsafe(wctxt))
> > > > +                     break;
> > > > +     }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 


Thanks & Regards,
Kartik

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[andriy.shevchenko]

On Thu, Feb 13, 2025 at 10:11:36AM +0000, Kartik Rajput wrote:
> On Thu, 2025-02-13 at 11:55 +0200, andriy.shevchenko@linux.intel.com
> wrote:
> > On Thu, Feb 13, 2025 at 09:05:36AM +0000, Kartik Rajput wrote:
> > > On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:

...

> > > > > +     for (i = 0; i < len; i++) {
> > > > > +             if (!nbcon_enter_unsafe(wctxt))
> > > > > +                     break;
> > > > > +
> > > > > +             read_poll_timeout_atomic(tegra_utc_tx_readl, val,
> > > > > !(val & TEGRA_UTC_FIFO_FULL),
> > > > > +                                      0, USEC_PER_SEC, false,
> > > > > tup,
> > > > > TEGRA_UTC_FIFO_STATUS);
> > > > 
> > > > No error check?
> > > 
> > > I'm not sure about this. The case where the TX FIFO doesn't clear
> > > up,
> > > even after polling for 1 second, is highly unlikely, especially
> > > since
> > > there's no flow control involved here. Even if that did happen,
> > > writing
> > > to the TX FIFO should just result in an overflow, which is probably
> > > acceptable in this scenario.
> > 
> > Perhaps a warning (debug?) message in such a case?
> 
> I would prefer avoiding any prints in this function, as we are writing
> debug messages to the UART HW here.

Not every printf() goes to UART, but to your point the NBCON should solve that
issue to some extent. Of course, if the HW in a broken (unrecoverable) state,
nothing won't help.

> > > > > +             uart_console_write(&tup->port, wctxt->outbuf + i,
> > > > > 1,
> > > > > tegra_utc_console_putchar);
> > > > > +
> > > > > +             if (!nbcon_exit_unsafe(wctxt))
> > > > > +                     break;
> > > > > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

On Thu, 2025-02-13 at 12:57 +0200, andriy.shevchenko@linux.intel.com
wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Feb 13, 2025 at 10:11:36AM +0000, Kartik Rajput wrote:
> > On Thu, 2025-02-13 at 11:55 +0200,
> > andriy.shevchenko@linux.intel.com
> > wrote:
> > > On Thu, Feb 13, 2025 at 09:05:36AM +0000, Kartik Rajput wrote:
> > > > On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> > > > > On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput
> > > > > wrote:
> 
> ...
> 
> > > > > > +     for (i = 0; i < len; i++) {
> > > > > > +             if (!nbcon_enter_unsafe(wctxt))
> > > > > > +                     break;
> > > > > > +
> > > > > > +             read_poll_timeout_atomic(tegra_utc_tx_readl,
> > > > > > val,
> > > > > > !(val & TEGRA_UTC_FIFO_FULL),
> > > > > > +                                      0, USEC_PER_SEC,
> > > > > > false,
> > > > > > tup,
> > > > > > TEGRA_UTC_FIFO_STATUS);
> > > > > 
> > > > > No error check?
> > > > 
> > > > I'm not sure about this. The case where the TX FIFO doesn't
> > > > clear
> > > > up,
> > > > even after polling for 1 second, is highly unlikely, especially
> > > > since
> > > > there's no flow control involved here. Even if that did happen,
> > > > writing
> > > > to the TX FIFO should just result in an overflow, which is
> > > > probably
> > > > acceptable in this scenario.
> > > 
> > > Perhaps a warning (debug?) message in such a case?
> > 
> > I would prefer avoiding any prints in this function, as we are
> > writing
> > debug messages to the UART HW here.
> 
> Not every printf() goes to UART, but to your point the NBCON should
> solve that
> issue to some extent. Of course, if the HW in a broken
> (unrecoverable) state,
> nothing won't help.
> 

I agree, but we still risk flooding up the ring buffer if we add debug
message here.

> > > > > > +             uart_console_write(&tup->port, wctxt->outbuf
> > > > > > + i,
> > > > > > 1,
> > > > > > tegra_utc_console_putchar);
> > > > > > +
> > > > > > +             if (!nbcon_exit_unsafe(wctxt))
> > > > > > +                     break;
> > > > > > +     }
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks & Regards,
Kartik