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@xxxxxxxxxxxxxxx>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks & Regards,
Kartik
