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

[Kartik Rajput <kkartik@nvidia.com>]

Hi Andy,

Thanks for the review!


On Wed, 2025-02-12 at 17:09 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Feb 12, 2025 at 04:11:32PM +0530, Kartik Rajput wrote:
> > The Tegra264 SoC supports the UART Trace Controller (UTC), 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.
> 
> ...
> 
> > +/*
> > + * NVIDIA Tegra UTC (UART Trace Controller) driver.
> > + */
> 
> Can be a single line.
> 

Ack.

> ...
> 
> > +#include <linux/bits.h>
> > +#include <linux/console.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> 
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> 
> iopoll.h guarantees to include io.h in case you want to have less
> lines here.
> (yeah, I know that the header guarantees is a tribal knowledge, it's
> undocumented)

Ack. I will remove io.h from here.

> 
> > +#include <linux/kfifo.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> 
> > +#include <linux/of.h>
> 
> Is this being used now?
> 

No, I will remove this.

> > +#include <linux/property.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/serial.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/types.h>
> 
> ...
> 
> > +#define UART_NR                                      16
> 
> Bad naming, calling for collisions. Move it to the driver's
> namespace.
> 

Ack.

> ...
> 
> > +static void tegra_utc_init_tx(struct tegra_utc_port *tup)
> > +{
> > +     /* Disable TX. */
> > +     tegra_utc_tx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
> > +
> > +     /* Update the FIFO Threshold. */
> > +     tegra_utc_tx_writel(tup, tup->tx_threshold,
> > TEGRA_UTC_FIFO_THRESHOLD);
> > +
> > +     /* Clear and mask all the interrupts. */
> > +     tegra_utc_tx_writel(tup, TEGRA_UTC_INTR_REQ |
> > TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
> > +                         TEGRA_UTC_INTR_CLEAR);
> 
> Here...
> 
> > +     tegra_utc_disable_tx_irq(tup);
> > +
> > +     /* Enable TX. */
> > +     tegra_utc_tx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE,
> > TEGRA_UTC_ENABLE);
> > +}
> > +
> > +static void tegra_utc_init_rx(struct tegra_utc_port *tup)
> > +{
> > +     tup->rx_irqmask = TEGRA_UTC_INTR_REQ |
> > TEGRA_UTC_INTR_TIMEOUT;
> > +
> > +     tegra_utc_rx_writel(tup, TEGRA_UTC_COMMAND_RESET,
> > TEGRA_UTC_COMMAND);
> > +     tegra_utc_rx_writel(tup, tup->rx_threshold,
> > TEGRA_UTC_FIFO_THRESHOLD);
> > +
> > +     /* Clear all the pending interrupts. */
> > +     tegra_utc_rx_writel(tup, TEGRA_UTC_INTR_TIMEOUT |
> > TEGRA_UTC_INTR_OVERFLOW |
> > +                         TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL
> > |
> > +                         TEGRA_UTC_INTR_EMPTY,
> > TEGRA_UTC_INTR_CLEAR);
> 
> ...and here the potential of deduplication by introducing an
> additional constant:
> 
> #define TEGRA_UTC_INTR_COMMON   \
>         (...)
> 
> (choose better name)
> 

Ack.

I think TEGRA_UTC_INTR_COMMON would be a good name for this macro since
these interrupts are common between TX and RX client.

> > +     tegra_utc_rx_writel(tup, tup->rx_irqmask,
> > TEGRA_UTC_INTR_MASK);
> > +     tegra_utc_rx_writel(tup, tup->rx_irqmask,
> > TEGRA_UTC_INTR_SET);
> > +
> > +     /* Enable RX. */
> > +     tegra_utc_rx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE,
> > TEGRA_UTC_ENABLE);
> > +}
> 
> ...
> 
> > +static bool tegra_utc_tx_chars(struct tegra_utc_port *tup)
> > +{
> > +     struct uart_port *port = &tup->port;
> > +     unsigned int pending;
> > +     u8 c;
> > +
> > +     pending = uart_port_tx(port, c,
> > +                  !(tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS)
> > & TEGRA_UTC_FIFO_FULL),
> > +                  tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA));
> 
> Make the last two to reside in temporary variables with self-
> explanatory names.
> 
> > +
> 
> Redundant blank line.

Ack.

> 
> > +     if (pending)
> > +             return true;
> > +
> > +     return false;
> 
>         return pending;
> 

Ack.

> > +}
> 
> ...
> 
> > +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(port->irq, tegra_utc_isr, 0, dev_name(port-
> > >dev), tup);
> 
> Seems the same Q stands about sharing, perhaps a comment why it's
> expected to
> be always exclusive?

Ack. I will drop a comment here.

> 
> > +     if (ret < 0)
> > +             dev_err(port->dev, "failed to register interrupt
> > handler\n");
> > +
> > +     return ret;
> > +}
> 
> ...
> 
> > +     for (i = 0; i < len; i++) {
> > +             if (!nbcon_enter_unsafe(wctxt))
> > +                     break;
> > +
> > +             read_poll_timeout_atomic(tegra_utc_tx_readl, val,
> > !(val & TEGRA_UTC_FIFO_FULL),
> > +                                      0, USEC_PER_SEC, false, tup,
> > TEGRA_UTC_FIFO_STATUS);
> 
> No error check?
> 

I'm not sure about this. The case where the TX FIFO doesn't clear up,
even after polling for 1 second, is highly unlikely, especially since
there's no flow control involved here. Even if that did happen, writing
to the TX FIFO should just result in an overflow, which is probably
acceptable in this scenario.

> > +             uart_console_write(&tup->port, wctxt->outbuf + i, 1,
> > tegra_utc_console_putchar);
> > +
> > +             if (!nbcon_exit_unsafe(wctxt))
> > +                     break;
> > +     }
> 
> > +
> 
> Unneeded blank line.

Ack.

> 
> > +}
> 
> ...
> 
> > +static int tegra_utc_probe(struct platform_device *pdev)
> > +{
> > +     const unsigned int *soc_fifosize;
> > +     struct device *dev = &pdev->dev;
> > +     struct tegra_utc_port *tup;
> > +     int ret;
> > +
> > +     tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
> > +     if (!tup)
> > +             return -ENOMEM;
> > +
> > +     ret = device_property_read_u32(dev, "tx-threshold", &tup-
> > >tx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing tx-threshold
> > device-tree property\n");
> 
> ' device-tree' is redundant part.

Ack.

> 
> > +     ret = device_property_read_u32(dev, "rx-threshold", &tup-
> > >rx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing rx-threshold
> > device-tree property\n");
> 
> Ditto.
> 
> Also in a form of
> 
>                 return dev_err_probe(dev, ret, "missing %s
> property\n", "rx-threshold");
> 
> in both cases the size of the object file will be smaller by a couple
> of dozens
> of bytes.

Ack.

> 
> > +     soc_fifosize = device_get_match_data(&pdev->dev);
> > +     tup->fifosize = *soc_fifosize;
> > +
> > +     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);
> > +
> > +     ret = tegra_utc_setup_port(&pdev->dev, tup);
> > +     if (ret)
> > +             dev_err_probe(dev, ret, "failed to setup uart
> > port\n");
> > +
> > +     platform_set_drvdata(pdev, tup);
> > +
> > +     return tegra_utc_register_port(tup);
> > +}
> 
> ...
> 
> > +static int __init tegra_utc_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = uart_register_driver(&tegra_utc_driver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = platform_driver_register(&tegra_utc_platform_driver);
> > +     if (ret) {
> > +             uart_unregister_driver(&tegra_utc_driver);
> 
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> 
> Just
> 
>         return ret;
> 
> will be good instead of the above 4 LoCs.
> 

Ack.

> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
>