Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752680AbdLHTq5 (ORCPT ); Fri, 8 Dec 2017 14:46:57 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:44380 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750908AbdLHTq4 (ORCPT ); Fri, 8 Dec 2017 14:46:56 -0500 X-Google-Smtp-Source: AGs4zMaUMFllLdj8qWdWYVhN+8s1g1leqkspnCIG9ITnJw4QSTXYllkKxwKrlK00zUSGA0mS0WKq/Q== Date: Fri, 08 Dec 2017 11:46:54 -0800 (PST) X-Google-Original-Date: Fri, 08 Dec 2017 11:42:19 PST (-0800) Subject: Re: [PATCH] tty: New RISC-V SBI console driver In-Reply-To: <20171208073718.GA14027@kroah.com> CC: jslaby@suse.com, patches@groups.riscv.org, linux-kernel@vger.kernel.org From: Palmer Dabbelt To: gregkh@linuxfoundation.org Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3428 Lines: 94 On Thu, 07 Dec 2017 23:37:18 PST (-0800), gregkh@linuxfoundation.org wrote: > On Thu, Dec 07, 2017 at 04:10:15PM -0800, Palmer Dabbelt wrote: >> From: Palmer Dabbelt >> >> The RISC-V ISA defines a simple console that is availiable via SBI calls >> on all systems. This patch adds a driver for this console interface >> that can act as both a target for early printk and as the system >> console. The core arch code already enables the early printk support >> when CONFIG_HVC_RISCV_SBI is defined. >> >> There is one checkpatch.pl warning here: to check the MAINTAINERS file. >> They're all matched by the "K: riscv" line. >> >> Signed-off-by: Palmer Dabbelt >> --- >> arch/riscv/include/asm/hvc_riscv_sbi.h | 12 ++++++ >> drivers/tty/hvc/Kconfig | 11 +++++ >> drivers/tty/hvc/Makefile | 1 + >> drivers/tty/hvc/hvc_riscv_sbi.c | 75 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 99 insertions(+) >> create mode 100644 arch/riscv/include/asm/hvc_riscv_sbi.h >> create mode 100644 drivers/tty/hvc/hvc_riscv_sbi.c >> >> diff --git a/arch/riscv/include/asm/hvc_riscv_sbi.h b/arch/riscv/include/asm/hvc_riscv_sbi.h >> new file mode 100644 >> index 000000000000..41723ed7bd97 >> --- /dev/null >> +++ b/arch/riscv/include/asm/hvc_riscv_sbi.h >> @@ -0,0 +1,12 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef _ASM_RISCV_HVC_RISCV_SBI_H >> +#define _ASM_RISCV_HVC_RISCV_SBI_H >> + >> +/* >> + * We always support CONFIG_EARLY_PRINTK via the SBI console driver because it >> + * works well enough that there's no penalty to doing so. >> + */ >> +extern struct console riscv_sbi_early_console_dev __initdata; > > Are you sure that __initdata should go into a .h file with an extern? > > And why do you need this .h file? Nothing in this patch uses it. I'm not sure we need it, but it's the best way I could come up with to get this into setup_arch for the early printk stuff. We use it in arch/riscv/kernel/setup.c, via #ifdef CONFIG_HVC_RISCV_SBI #include #endif and void __init setup_arch(char **cmdline_p) { #if defined(CONFIG_HVC_RISCV_SBI) if (likely(early_console == NULL)) { early_console = &riscv_sbi_early_console_dev; register_console(early_console); } #endif I don't know how I missed this the first time, but I can actually just move all that to setup.c, as it's calling directly into the SBI anyway. I think the cleanest way to support this would be to: * Submit a 4.15 cleanup to remove those two blocks above that call into this driver from our arch code. They can't be compiled in anyway, so it should be safe. * Submit a v2 of this patch without any of the early printk stuff. I'd assumed this patch would target the 4.16 merge window already, so as long as the first patch is in I think it should be safe. * Submit a patch that adds support for early printk. This will depend on the first patch, but won't depend on this driver at all as it can just call directly into the SBI. Sorry this is a mess, I'm still a bit new at this. > >> +config HVC_RISCV_SBI >> + bool "RISC-V SBI console support" >> + depends on RISCV >> + select HVC_DRIVER >> + default y > > Unless you can not boot your machine without this, you do not need 'y'. OK. > > thanks, > > greg k-h