Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753322AbbEZRMl (ORCPT ); Tue, 26 May 2015 13:12:41 -0400 Received: from mail-qk0-f177.google.com ([209.85.220.177]:33739 "EHLO mail-qk0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752283AbbEZRMi (ORCPT ); Tue, 26 May 2015 13:12:38 -0400 Message-ID: <5564A982.9050600@hurleysoftware.com> Date: Tue, 26 May 2015 13:12:34 -0400 From: Peter Hurley User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Bin Gao , Greg Kroah-Hartman CC: Jiri Slaby , One Thousand Gnomes , linux-kernel@vger.kernel.org, "linux-serial@vger.kernel.org" Subject: Re: [PATCH v4 1/2] serial_core: add pci uart early console support References: <20150522160659.GA108447@worksta> In-Reply-To: <20150522160659.GA108447@worksta> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10612 Lines: 361 Hi Bin, Please don't drop lists (or other addressees) from patch revisions. [ +cc linux-serial] On 05/22/2015 12:06 PM, Bin Gao wrote: > On some Intel Atom SoCs, the legacy IO port UART(0x3F8) is not available. > Instead, a 8250 compatible PCI uart can be used as early console. > This patch adds pci support to the 8250 early console driver uart8250. > For example, to enable pci uart(00:21.3) as early console on these > platforms, append the following line to the kernel command line > (assume baud rate is 115200): > earlyprintk=uart8250,pci32,0:24.2,115200n8 > > Signed-off-by: Bin Gao > --- > Changes in v4: > - moved PCI_EARLY definition from arch/x86/Kconfig to drivers/pci/Kconfig > - added 'earlyprintk' for x86 as alias to the early param 'earlycon' > arch/x86/Kconfig | 1 + > drivers/pci/Kconfig | 11 +++ > drivers/tty/serial/earlycon.c | 9 +++ > drivers/tty/serial/serial_core.c | 145 ++++++++++++++++++++++++++++++++++++++- Please update Documentation/kernel-parameters.txt with pci-specific earlycon parameter formats. > 4 files changed, 164 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 226d569..bdedd61 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -143,6 +143,7 @@ config X86 > select ACPI_LEGACY_TABLES_LOOKUP if ACPI > select X86_FEATURE_NAMES if PROC_FS > select SRCU > + select PCI_EARLY if PCI > > config INSTRUCTION_DECODER > def_bool y > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig > index 7a8f1c5..4f0f055 100644 > --- a/drivers/pci/Kconfig > +++ b/drivers/pci/Kconfig > @@ -114,4 +114,15 @@ config PCI_LABEL > def_bool y if (DMI || ACPI) > select NLS > > +config PCI_EARLY > + bool "Early PCI access" > + depends on PCI > + default n > + help > + This option indicates that a group of APIs are available (in > + asm/pci-direct.h) so the kernel can access pci config registers > + before the PCI subsystem is initialized. Any arch that supports > + early pci APIs should enable this option which is required by > + arch independent codes, e.g. uart8250 pci early console driver. > + > source "drivers/pci/host/Kconfig" > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > index 6dc471e..63ae60e 100644 > --- a/drivers/tty/serial/earlycon.c > +++ b/drivers/tty/serial/earlycon.c > @@ -193,6 +193,15 @@ static int __init param_setup_earlycon(char *buf) > } > early_param("earlycon", param_setup_earlycon); > > +/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */ > +#ifdef CONFIG_X86 > +static int __init param_setup_earlycon_x86(char *buf) > +{ > + return param_setup_earlycon(buf); > +} > +early_param("earlyprintk", param_setup_earlycon_x86); > +#endif > + Please move this hunk to patch 2/2. FYI, there is a proposal to evaluate "earlyprintk=" earlier than parse_early_params(), which this would break. https://lkml.org/lkml/2015/5/18/127 > int __init of_setup_earlycon(unsigned long addr, > int (*setup)(struct earlycon_device *, const char *)) > { > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 0b7bb12..19ca2a0 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -34,10 +34,15 @@ > #include > #include > #include > +#include > > #include > #include > > +#ifdef CONFIG_PCI_EARLY > +#include > +#endif > + > /* > * This is used to lock changes in serial line configuration. > */ > @@ -1808,6 +1813,110 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co) > return ports + idx; > } > > +static int parse_bdf(char *options, char **endp, char delimiter, u8 *val) If this function is only used from parse_pci_options(), please enclose it in #ifdef CONFIG_PCI_EARLY scope with parse_pci_options(). > +{ > + char str[4]; /* max 3 chars, plus a NULL terminator */ > + char *p = options; > + int i = 0; > + > + while (*p) { > + if (i >= 4) > + return -EINVAL; > + > + if (*p == delimiter) { > + str[i++] = 0; > + if (endp) > + *endp = p + 1; > + return kstrtou8(str, 10, val); /* decimal, no hex */ > + } > + > + str[i++] = *p++; > + } Is all this to avoid using simple_strtoul()? If yes, I'd rather you use simple_strtoul() like the rest of the console code and ignore the (misguided) advice that simple_strtoul() is obsolete. The code above is exactly what is wrong with the kstrto* api. > + > + return -EINVAL; > +} > + > +#ifdef CONFIG_PCI_EARLY > +/* > + * The whole pci option from the command line is: pci[32],B:D.F[,options] > + * Examples: > + * pci,0:21.3,115200n8 > + * pci32,0:21.3 > + * Here pci32 means 8250 UART registers are 32-bit width(regshift = 2). > + * pci means 8250 UART registers are 8-bit width(regshift = 0). > + * B,D and F are bus, device and function, in decimal(not hex). > + * The additional options(115200n8) would be parsed by the earlycon framework. > + * > + * @options: the pci options > + * @phys: the pointer to return pci mem or io address > + * return: <0: error > + * 0: pci mem > + * 1: pci io > + */ > +static int parse_pci_options(char *options, unsigned long *phys) > +{ > + u8 bus, dev, func; > + char *endp; > + u64 bar0; > + u16 cmd; > + int pci_io = 0; > + > + if (!early_pci_allowed()) { > + pr_err("earlycon pci not available(early pci not allowed)\n"); Error message is redundant. > + return -EINVAL; > + } > + > + /* We come here with options=B:D.F[,options] */ > + if (parse_bdf(options, &endp, ':', &bus)) > + goto failed; > + > + if (parse_bdf(endp, &endp, '.', &dev)) > + goto failed; > + > + if (parse_bdf(endp, &endp, ',', &func)) > + goto failed; > + > + /* > + * On these platforms class code in pci config is broken, ^^^^^^^^^^^^^^^ which platforms? > + * so skip checking it. > + */ > + > + bar0 = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0); > + > + /* The BAR is IO or Memory? */ > + if ((bar0 & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) > + pci_io = 1; > + > + if ((bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > + PCI_BASE_ADDRESS_MEM_TYPE_64) > + bar0 |= (u64)read_pci_config(bus, dev, func, > + PCI_BASE_ADDRESS_0 + 4) << 32; > + > + *phys = bar0 & (pci_io ? PCI_BASE_ADDRESS_IO_MASK : > + PCI_BASE_ADDRESS_MEM_MASK); > + > + /* Enable address decoding */ > + cmd = read_pci_config_16(bus, dev, func, PCI_COMMAND); > + write_pci_config_16(bus, dev, func, PCI_COMMAND, > + cmd | (pci_io ? PCI_COMMAND_IO : PCI_COMMAND_MEMORY)); > + > + pr_info("Use 8250 uart at PCI 0000:%02u:%02u.%01u as early console\n", > + bus, dev, func); I think one earlycon banner is sufficient; you could make this pr_debug() instead. Existing convention for earlycon messages is "earlycon: ...." > + return pci_io; > + > +failed: > + pr_err("Invalid earlycon pci parameters\n"); > + return -EINVAL; > +} > + > +#else > +static int parse_pci_options(char *options, unsigned long *phys) > +{ > + pr_err("earlycon pci not available(need CONFIG_PCI_EARLY)\n"); "earlycon: PCI not supported (requires CONFIG_PCI_EARLY=y)" ? > + return -EINVAL; > +} > +#endif > + > /** > * uart_parse_earlycon - Parse earlycon options > * @p: ptr to 2nd field (ie., just beyond ',') > @@ -1816,8 +1925,9 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co) > * @options: ptr for field; NULL if not present (out) > * > * Decodes earlycon kernel command line parameters of the form > - * earlycon=,io|mmio|mmio32,, > + * earlycon=,io|mmio|mmio32|pci|pci32,, Document the pci/pci32 format separately because earlycon=uart8250,pci32,, is not a valid form. > * console=,io|mmio|mmio32,, > + * For pci/pci32, the format is B:D.F, e.g. 0:24.2 > * > * The optional form > * earlycon=,0x, > @@ -1829,12 +1939,23 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co) > int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, > char **options) > { > + int pci = 0, ret; > + unsigned long phys; > + > if (strncmp(p, "mmio,", 5) == 0) { > *iotype = UPIO_MEM; > p += 5; > } else if (strncmp(p, "mmio32,", 7) == 0) { > *iotype = UPIO_MEM32; > p += 7; > + } else if (strncmp(p, "pci,", 4) == 0) { > + pci = 1; > + p += 4; > + ret = parse_pci_options(p, &phys); > + } else if (strncmp(p, "pci32,", 6) == 0) { > + pci = 2; > + p += 6; > + ret = parse_pci_options(p, &phys); > } else if (strncmp(p, "io,", 3) == 0) { > *iotype = UPIO_PORT; > p += 3; > @@ -1844,7 +1965,27 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, > return -EINVAL; > } > > - *addr = simple_strtoul(p, NULL, 0); > + if (pci) { > + if (ret < 0) /* error */ > + return ret; > + > + /* > + * Once PCI mem/io is read from PCI BAR, we can reuse > + * mmio/mmio32/io type to minimize code change. > + */ > + if (ret > 0) /* PCI io */ > + *iotype = UPIO_PORT; > + else { /* ret = 0: PCI mem */ > + if (pci == 2) > + *iotype = UPIO_MEM32; > + else > + *iotype = UPIO_MEM; > + } > + > + *addr = phys; > + } else > + *addr = simple_strtoul(p, NULL, 0); > + I'd like to see this refactored without the logic steering locals. Like this: if (strncmp(p, "mmio,", 5) == 0) { *iotype = UPIO_MEM; p += 5; + *addr = simple_strtoul(p, NULL, 0); } else if (strncmp(p, "mmio32,", 7) == 0) { *iotype = UPIO_MEM32; p += 7; + *addr = simple_strtoul(p, NULL, 0); + } else if (strncmp(p, "pci,", 4) == 0) { + pci = 1; + p += 4; + ret = parse_pci_options(p, addr); + if (ret < 0) + return ret; + *iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM; + } else if (strncmp(p, "pci32,", 6) == 0) { + pci = 2; + p += 6; + ret = parse_pci_options(p, addr); + if (ret < 0) + return ret; + *iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM32; } else if (strncmp(p, "io,", 3) == 0) { *iotype = UPIO_PORT; p += 3; + *addr = simple_strtoul(p, NULL, 0); Regards, Peter Hurley > - *addr = simple_strtoul(p, NULL, 0); > p = strchr(p, ','); > if (p) > p++; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/