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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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