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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2cdba410-7cc8-6ad3-53ab-d9c24e58a028@kapsi.fi>
Date:   Mon, 15 Feb 2021 12:35:31 +0200
From:   Mikko Perttunen <cyndis@...si.fi>
To:     Thierry Reding <thierry.reding@...il.com>,
        Mikko Perttunen <mperttunen@...dia.com>
Cc:     gregkh@...uxfoundation.org, jirislaby@...nel.org,
        jonathanh@...dia.com, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] tty: serial: Add earlycon driver for Tegra Combined UART

On 2/15/21 12:25 PM, Thierry Reding wrote:
> 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.

It's that the name of the driver 'tegra_comb_uart' matches what the boot 
chain passes; and that OF_EARLYCON_DECLARE is not used. 
(OF_EARLYCON_DECLARE cannot anyway be used due to the mailbox 
indirection in device tree).

> 
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@...dia.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.

True, I'll remove it.

> 
>> +#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.

This is very similar to the main tegra_tcu driver, but considering how 
simple this driver is, and the main driver using the mailbox framework 
making the actual implementation incompatible, I was thinking that it's 
easier to just have this be independent.

> 
> Thierry
> 

Thanks,
Mikko