Re: [PATCH v2 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)
From: Andy Shevchenko
Date: Tue Feb 11 2025 - 05:01:30 EST
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