| Message ID | 20210213115824.3306965-1-mperttunen@nvidia.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | tty: serial: Add earlycon driver for Tegra Combined UART | expand |
Hi Mikko,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on tegra/for-next linux/master linus/master v5.11-rc7 next-20210212]
[cannot apply to tty/tty-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mikko-Perttunen/tty-serial-Add-earlycon-driver-for-Tegra-Combined-UART/20210213-200425
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm-randconfig-r016-20210213 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/889104c3001a64bab09c4ac82aed58f1d98ec77a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mikko-Perttunen/tty-serial-Add-earlycon-driver-for-Tegra-Combined-UART/20210213-200425
git checkout 889104c3001a64bab09c4ac82aed58f1d98ec77a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/tegra-tcu-earlycon.c:62:12: warning: no previous prototype for function 'early_tegra_combined_uart_setup' [-Wmissing-prototypes]
int __init early_tegra_combined_uart_setup(struct earlycon_device *device, const char *options)
^
drivers/tty/serial/tegra-tcu-earlycon.c:62:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __init early_tegra_combined_uart_setup(struct earlycon_device *device, const char *options)
^
static
1 warning generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for ARM_CPU_SUSPEND
Depends on ARCH_SUSPEND_POSSIBLE
Selected by
- ARM_QCOM_SPM_CPUIDLE && CPU_IDLE && (ARM || ARM64) && (ARCH_QCOM || COMPILE_TEST && !ARM64
vim +/early_tegra_combined_uart_setup +62 drivers/tty/serial/tegra-tcu-earlycon.c
61
> 62 int __init early_tegra_combined_uart_setup(struct earlycon_device *device, const char *options)
63 {
64 if (!(device->port.membase))
65 return -ENODEV;
66
67 device->con->write = early_tcu_write;
68
69 return 0;
70 }
71
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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
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@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. 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
On Mon, Feb 15, 2021 at 12:35:31PM +0200, Mikko Perttunen wrote: > 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). This is all not immediately obvious. Perhaps you can add more of this into the commit message and perhaps provide an example of how this would be used on the kernel command-line. You say "mailbox indirection" and looking at the implementation this does seem to use the mailbox's base address as a sort of TX FIFO, which I think is all good. However, I'm wondering if we couldn't somehow detect this all dynamically at runtime. Don't we have access to the device tree node at this point? If so, couldn't we parse all the necessary information from the DT instead of relying on the user providing the mailbox address on the command-line? I realize that this would all make things a bit more complicated in this driver, but at the same time it'd make life so much easier for users, so I think it's worth at least considering. To elaborate on this a bit, I think it'd be much more useful if users could specify something like this: earlycon=tegra-tcu rather than: earlycon=tegra_comb_uart,0xc150000 Note that I'm not even sure if that's a correct address. It'd be even better if all of this can just be derived from the device tree. My recollection is that earlycon always needs to be explicitly enabled, but I thought it was also possible to derive which console to use from the /chose/stdout-path property in device tree. > > > 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. > > 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. I don't have a strong objection to keeping these functions separate, especially since they are fairly small and not likely to ever change, so the maintenance burden is going to be small in any case. But even so it might be nice to stash this all into the same file. After all, people aren't going to enable this configuration option if they have the Tegra TCU driver disabled. Once these are integrated, it's also likely not worth even having a separate Kconfig option because the added code is so little. Thierry
On 2/15/21 2:09 PM, Thierry Reding wrote: > On Mon, Feb 15, 2021 at 12:35:31PM +0200, Mikko Perttunen wrote: >> 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). > > This is all not immediately obvious. Perhaps you can add more of this > into the commit message and perhaps provide an example of how this would > be used on the kernel command-line. Will do. > > You say "mailbox indirection" and looking at the implementation this > does seem to use the mailbox's base address as a sort of TX FIFO, which > I think is all good. However, I'm wondering if we couldn't somehow > detect this all dynamically at runtime. Don't we have access to the > device tree node at this point? If so, couldn't we parse all the > necessary information from the DT instead of relying on the user > providing the mailbox address on the command-line? Sure, I will look at parsing the address from DT manually at init time. > > I realize that this would all make things a bit more complicated in this > driver, but at the same time it'd make life so much easier for users, so > I think it's worth at least considering. > > To elaborate on this a bit, I think it'd be much more useful if users > could specify something like this: > > earlycon=tegra-tcu > > rather than: > > earlycon=tegra_comb_uart,0xc150000 > > Note that I'm not even sure if that's a correct address. It'd be even > better if all of this can just be derived from the device tree. My > recollection is that earlycon always needs to be explicitly enabled, but > I thought it was also possible to derive which console to use from the > /chose/stdout-path property in device tree. The reason I don't think this is that complicated is that that is what cboot is already passing to the kernel. Maybe we could support both options since it's just 2 or 3 extra lines. > >>>> 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. >> >> 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. > > I don't have a strong objection to keeping these functions separate, > especially since they are fairly small and not likely to ever change, so > the maintenance burden is going to be small in any case. > > But even so it might be nice to stash this all into the same file. After > all, people aren't going to enable this configuration option if they > have the Tegra TCU driver disabled. Once these are integrated, it's also > likely not worth even having a separate Kconfig option because the added > code is so little. Will look into integrating this into tegra-tcu.c. > > Thierry > Thanks! Mikko
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 confirm that this patch applies mostly cleanly (modulo minor Makefile issue) to v5.16 and that it works with the vendor cboot on a Xavier AGX device. I assume that it is expected that the earlycon output comes out twice (once from the earlycon driver and another time from the real driver). Tested-by: Helmut Grohne <helmut.grohne@intenta.de> Some of the concerns from Thierry Reding remain unaddressed though. Helmut
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 +#define INTR_TRIGGER_BIT 31 + +static u32 update_and_send_mbox(u8 __iomem *addr, u32 mbox_val, char c) +{ + int bytes = bytes = (mbox_val >> NUM_BYTES_FIELD_BIT) & 0x3; + + mbox_val |= BIT(INTR_TRIGGER_BIT); + mbox_val |= c << (bytes * 8); + bytes++; + mbox_val = (mbox_val & ~(3 << NUM_BYTES_FIELD_BIT)) | + (bytes << NUM_BYTES_FIELD_BIT); + + if (bytes == 3) { + /* Send current packet to SPE */ + while (readl(addr) & BIT(INTR_TRIGGER_BIT)) + cpu_relax(); + writel(mbox_val, addr); + mbox_val = BIT(INTR_TRIGGER_BIT); + } + + return mbox_val; +} + +/* + * 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); + } +} + +int __init early_tegra_combined_uart_setup(struct earlycon_device *device, const char *options) +{ + if (!(device->port.membase)) + return -ENODEV; + + device->con->write = early_tcu_write; + + return 0; +} + +EARLYCON_DECLARE(tegra_comb_uart, early_tegra_combined_uart_setup);
Add an earlycon driver for platforms with TCU, namely Tegra194. The driver is compatible with boot parameters passed by NVIDIA boot chains. 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