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

[Kartik Rajput <kkartik@nvidia.com>]

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