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?
...
> +#define TEGRA_UTC_ENABLE 0x0
It would be nice to use fixed width values for the register offsets,
e.g., 0x000 here.
> +#define TEGRA_UTC_ENABLE_CLIENT_ENABLE BIT(0)
> +
> +#define TEGRA_UTC_FIFO_THRESHOLD 0x8
> +
> +#define TEGRA_UTC_COMMAND 0xc
Ditto.
> +#define TEGRA_UTC_COMMAND_RESET BIT(0)
> +#define TEGRA_UTC_COMMAND_FLUSH BIT(1)
> +#define TEGRA_UTC_DATA 0x20
Ditto.
> +#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?
> +#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?
> + 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);
> + 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.
> + 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
