Message-ID: <974ae61f-6883-40fb-b5b1-27139c0f07df@kernel.org>
Date: Tue, 11 Feb 2025 08:23:47 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Kartik Rajput <kkartik@...dia.com>, gregkh@...uxfoundation.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 thierry.reding@...il.com, jonathanh@...dia.com, hvilleneuve@...onoff.com,
 arnd@...nel.org, geert+renesas@...der.be, robert.marko@...tura.hr,
 schnelle@...ux.ibm.com, andriy.shevchenko@...ux.intel.com,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
 devicetree@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v2 2/2] serial: tegra-utc: Add driver for Tegra UART Trace
 Controller (UTC)

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@...dia.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

Powered by blists - more mailing lists