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@xxxxxxxxxx>
>
> > --- /dev/null
> > +++ b/drivers/tty/serial/tegra-utc.c
> > @@ -0,0 +1,622 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION &
> > AFFILIATES. All rights reserved.
> > +/*
> > + * NVIDIA Tegra UTC (UART Trace Controller) driver.
> > + */
> > +
> > +#include <linux/console.h>
> > +#include <linux/kthread.h>
>
> Do you really use kthread somewhere?
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
