Re: [PATCH] tty: serial: Add earlycon driver for Tegra Combined UART

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On Sat, Feb 13, 2021 at 01:58:24PM +0200, Mikko Perttunen wrote:
> Add an earlycon driver for platforms with TCU, namely Tegra194.
> The driver is compatible with boot parameters passed by NVIDIA
> boot chains.

I'm not sure I understand the latter part of this description. What boot
parameters is this compatible with? Looking at the setup function there
doesn't seem to be anything out of the ordinary here, so I'm wondering
if that's just confusing. If there's anything special, it might be worth
specifically pointing out what that is. Perhaps both in the commit
message and in a code comment, so it's properly documented.

> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/tty/serial/Kconfig              | 12 +++++
>  drivers/tty/serial/Makefile             |  1 +
>  drivers/tty/serial/tegra-tcu-earlycon.c | 72 +++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>  create mode 100644 drivers/tty/serial/tegra-tcu-earlycon.c
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 34a2899e69c0..d941785e3f46 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -331,6 +331,18 @@ config SERIAL_TEGRA_TCU_CONSOLE
>  
>  	  If unsure, say Y.
>  
> +config SERIAL_TEGRA_TCU_EARLYCON
> +	bool "Earlycon on NVIDIA Tegra Combined UART"
> +	depends on ARCH_TEGRA || COMPILE_TEST
> +	select SERIAL_EARLYCON
> +	select SERIAL_CORE_CONSOLE
> +	default y if SERIAL_TEGRA_TCU_CONSOLE
> +	help
> +	  If you say Y here, TCU output will be supported during the earlycon
> +	  phase of the boot.
> +
> +	  If unsure, say Y.
> +
>  config SERIAL_MAX3100
>  	tristate "MAX3100 support"
>  	depends on SPI
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index b85d53f9e9ff..408144326fed 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_SERIAL_XILINX_PS_UART) += xilinx_uartps.o
>  obj-$(CONFIG_SERIAL_SIRFSOC) += sirfsoc_uart.o
>  obj-$(CONFIG_SERIAL_TEGRA) += serial-tegra.o
>  obj-$(CONFIG_SERIAL_TEGRA_TCU) += tegra-tcu.o
> +obj-$(CONFIG_SERIAL_TEGRA_TCU_EARLYCON) += tegra-tcu-earlycon.o
>  obj-$(CONFIG_SERIAL_AR933X)   += ar933x_uart.o
>  obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o
>  obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
> diff --git a/drivers/tty/serial/tegra-tcu-earlycon.c b/drivers/tty/serial/tegra-tcu-earlycon.c
> new file mode 100644
> index 000000000000..9decfbced0a7
> --- /dev/null
> +++ b/drivers/tty/serial/tegra-tcu-earlycon.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2021, NVIDIA CORPORATION.  All rights reserved.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/io.h>
> +#include <linux/serial_core.h>
> +
> +#define NUM_BYTES_FIELD_BIT	24
> +#define FLUSH_BIT		26

This one seems to be unused.

> +#define INTR_TRIGGER_BIT	31

I wonder if this could somehow be integrated with the existing TCU
driver since we have these bits defined there already. And really this
is basically a skeleton version of the same driver.

> +/*
> + * This function splits the string to be printed (const char *s) into multiple
> + * packets. Each packet contains a max of 3 characters. Packets are sent to the
> + * SPE-based combined UART server for printing. Communication with SPE is done
> + * through mailbox registers which can generate interrupts for SPE.
> + */
> +static void early_tcu_write(struct console *console, const char *s, unsigned int count)
> +{
> +	struct earlycon_device *device = console->data;
> +	u8 __iomem *addr = device->port.membase;
> +	u32 mbox_val = BIT(INTR_TRIGGER_BIT);
> +	unsigned int i;
> +
> +	/* Loop for processing each 3 char packet */
> +	for (i = 0; i < count; i++) {
> +		if (s[i] == '\n')
> +			mbox_val = update_and_send_mbox(addr, mbox_val, '\r');
> +		mbox_val = update_and_send_mbox(addr, mbox_val, s[i]);
> +	}
> +
> +	if ((mbox_val >> NUM_BYTES_FIELD_BIT) & 0x3) {
> +		while (readl(addr) & BIT(INTR_TRIGGER_BIT))
> +			cpu_relax();
> +		writel(mbox_val, addr);
> +	}
> +}

For example this function already exists in the Tegra TCU driver and
perhaps some of that could be refactored to work for both cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]