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

[Kartik Rajput <kkartik@nvidia.com>]

On Thu, 2025-02-13 at 13:05 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Feb 13, 2025 at 03:34:42PM +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.
> 
> Btw, neither the commit message nor cover letter explain why the new
> driver
> is needed. There are some serial Tegra drivers already. Is this one
> completely
> different from the existing drivers?
> 

Tegra264 platforms have a single debug UART which is shared across
multiple firmwares and the OS. The Tegra UTC is added to address this
issue.

It is a completely different driver.

> ...
> 
> > +#define TEGRA_UTC_ENABLE                     0x0
> 
> It would be nice to use fixed width values for the register offsets,
> e.g., 0x000 here.
> 

Ack.

> > +#define TEGRA_UTC_ENABLE_CLIENT_ENABLE               BIT(0)
> > +
> > +#define TEGRA_UTC_FIFO_THRESHOLD             0x8
> > +
> > +#define TEGRA_UTC_COMMAND                    0xc
> 
> Ditto.
> 

Ack.

> > +#define TEGRA_UTC_COMMAND_RESET                      BIT(0)
> > +#define TEGRA_UTC_COMMAND_FLUSH                      BIT(1)
> 
> > +#define TEGRA_UTC_DATA                               0x20
> 
> Ditto.
> 

Ack.

> > +#define TEGRA_UTC_FIFO_STATUS                        0x100
> > +#define TEGRA_UTC_FIFO_EMPTY                 BIT(0)
> > +#define TEGRA_UTC_FIFO_FULL                  BIT(1)
> > +#define TEGRA_UTC_FIFO_REQ                   BIT(2)
> > +#define TEGRA_UTC_FIFO_OVERFLOW                      BIT(3)
> > +#define TEGRA_UTC_FIFO_TIMEOUT                       BIT(4)
> > +
> > +#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_EMPTY                 BIT(0)
> > +#define TEGRA_UTC_INTR_FULL                  BIT(1)
> > +#define TEGRA_UTC_INTR_REQ                   BIT(2)
> > +#define TEGRA_UTC_INTR_OVERFLOW                      BIT(3)
> > +#define TEGRA_UTC_INTR_TIMEOUT                       BIT(4)
> 
> ...
> 
> > +#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
> > +#define TEGRA_UTC_DEFAULT_FIFO_THRESHOLD     0x4
> 
> Hmm... Is this a register offset? If not, why it's in a hexadecimal
> format?
> 

This is not a register offset. I will use a decimal value here.

> > +#define TEGRA_UTC_EARLYCON_MAX_BURST_SIZE    128
> 
> ...
> 
> > +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);
> 
> Use dev?

Thanks for catching this, this should've been done before. I will
update this.

> 
> > +     if (!tup)
> > +             return -ENOMEM;
> > +
> > +     ret = device_property_read_u32(dev, "tx-threshold", &tup-
> > >tx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing %s
> > property\n", "tx-threshold");
> > +
> > +     ret = device_property_read_u32(dev, "rx-threshold", &tup-
> > >rx_threshold);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "missing %s
> > property\n", "rx-threshold");
> > +
> > +     soc_fifosize = device_get_match_data(&pdev->dev);
> > +     

This needs update as well.

> > 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);
> 
> Ditto.

Ack.

> 
> > +     if (ret)
> > +             dev_err_probe(dev, ret, "failed to setup uart
> > port\n");
> > +
> > +     platform_set_drvdata(pdev, tup);
> > +
> > +     return tegra_utc_register_port(tup);
> > +}
> 
> ...
> 
> With the above being addressed, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks & Regards,
Kartik