| Message ID | 1453722324-22407-3-git-send-email-aleksey.makarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 01/28/2016 03:45 AM, Peter Hurley wrote: > On 01/27/2016 05:57 AM, Aleksey Makarov wrote: >> >> >> On 01/25/2016 07:32 PM, Peter Hurley wrote: >>> On 01/25/2016 03:45 AM, Aleksey Makarov wrote: >>>> 'ARM Server Base Boot Requiremets' [1] mention SPCR (Serial Port >>>> Console Redirection Table) [2] as a mandatory ACPI table that >>>> specifies the configuration of serial console. >>>> >>>> Parse this table and check if any registered console match the >>>> description. If it does, enable that console. >>>> >>>> To implement that, introduce a new member int (*acpi_match)(struct >>>> console *, struct acpi_table_spcr *) of struct console. It allows >>>> drivers to check if they provide a matching console device. >>> >>> Many, many platform proms with all sorts of binary table layout are >>> already supported by the existing console infrastructure. Why is ACPI >>> different, that requires extensive (and messy) changes to console >>> initialization? >> >> Without this patch, when linux calls register_console(), that function >> checks if any console has been enabled so far. 1) If not, it enables the >> console being registered. 2) If there exists any enabled console, it >> looks at the console_cmdline array. That array holds a list of >> consoles that user wishes to enable. There are two ways to append >> an item to that list: first is to pass "console=..." option in command >> line and second is to call add_preferred_console(char *name, int idx, >> char *options). As it is clear from the signature, the function >> requires the name of the driver (like "ttyS") and the line id. On the >> other hand, the SPCR ACPI table describes console by specifying the >> address of it's registers or PCI Device ID / PCI Vendor ID or PCI Bus >> Number / PCI Device Number. So to use this function we would need to >> have a method to translate this info to the name of terminal and line >> index. I could not figure out any way to do that. > > I'm not sure how this answers my question. It does not. For the actual answer please scroll down. > Which existing drivers/arch setup have you studied to conclude that > the existing console mechanisms don't work? I studied how add_preferred_console() is used in drivers/of/base.c, grepped the tree for it and studied each instance. Almost everything I see is dumb calls to this function with hardcoded tty name and line index. I don't see how this can help. > Have you actually looked > at the in-tree callers of add_preferred_console()? Yes I have. It looks like you are implying that there is some drivers/arch setup that would help to implement this correctly. Could you please give me a reference to it? >> In the initial version of the patch after getting the reference to the >> SPCR ACPI table the full tree of ACPI devices was searched to find any >> device with the same address. When uart_add_one_port() was called >> to register a new serial port, the ACPI companion of this port was >> compared to the found device. If it was the same device, the code >> called add_preferred_console() (the terminal name and line index are >> known in uart_add_one_port()). > > Yeah, I wasn't a fan of that. > > But I think it was a bad choice to pick SPCR as table format, in the > first place. At least DBG2 has the actual ACPI device identifier :/ I am working on parsing DBG2 to implement earlycon based on info from it. As I understand, - SPCR specifies which existing console should be enabled (= made preferred). - DBG2 specifies how kernel can add a new earlycon to the system. These are two differend subsystems and both make sense. Also I don't understand how having (optional) ACPI device identifier in DBG2 table can help to implement earlycon introduction. >> This original approach had two problems: >> >> 1) It works with the SPCR tables that describe consoles only by >> the address of the registers. I do not think that consoles that are >> described by PCI info will appear in the near future, but decided to >> implement this in a generic way. I would like to discuss if this >> decision was good. >> >> 2) Wrong order of initialization. Many console drivers have already >> been registered by the time uart_add_one_port() adds an item to the >> console_cmdline array. There is a similar problem with my >> implementation, but having a dedicated acpi_match() callback I >> believe made it simpler to circumwent. > > I don't see how the "wrong order of initialization" and the need for > acpi_match() correlate. What do you mean by "wrong order"? What is the > "right order"? There is a race between initialization/registration of consoles and parsing SPCR table. By the time the console is registered we should have SPCR table parsed and add_preferred_console() called. In the original submission add_preferred_console() was called from uart registration code, that requires to parse ACPI table very early (and nobody can say how early, because uarts can be registered very early. pl011 is initialized with arch_initcall() for example). I decided to fix this with retrying registration of consoles after ACPI is parsed. I do not like it either, but it is the simplest solution I could suggest. But it's quite difficult do implement such deferring code in the uart registration and, more important, it is wrong as there may be non-uart consoles. So acpi_match() is a part of registration retrying mechanism and the right place for it is at console registration. >> That's why I believe we need to add a new funcion pointer to struct >> console. On the other hand, I do not understand which existing >> structure you are referring. >> >>> How is this going to work with earlycon? >> >> If an earlycon that matches SPCR is being registered, the code will enable it. > > I think you should review how and when an earlycon is specified, initialized > and registered before you conclude that this will magically work. I reviewed that again and the only issue that I can imagine is that ACPI can not make earlycon preferred. Is it that you are afraid of? I believe it should not. What problems do you see? Thank you Aleksey Makarov >> While it is harmless. Even so I will check for earlycon in the next version >> of the patch set, thank you. >> >>> This commit log is missing the reasoning behind adding locks, >>> refactoring into delete_from_console_list(), and retry loops. >> >> I will add this to the next verion of the series. >> >> Thank you >> Aleksey Makarov >> >> >>>> [1] >>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html >>>> >>>> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx >>>> >>>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> >> >> [ .. ] >> >
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 573bebc..bf31e3c 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -4,6 +4,7 @@ config ARM64 select ACPI_GENERIC_GSI if ACPI select ACPI_PCI_HOST_GENERIC if ACPI select ACPI_REDUCED_HARDWARE_ONLY if ACPI + select ACPI_SPCR_TABLE if ACPI select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index e315061..142a338 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -60,6 +60,9 @@ config ACPI_CCA_REQUIRED config IORT_TABLE bool +config ACPI_SPCR_TABLE + bool + config ACPI_DEBUGGER bool "AML debugger interface (EXPERIMENTAL)" select ACPI_DEBUG diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 265eb90..8316859 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o obj-$(CONFIG_ACPI_BGRT) += bgrt.o obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o obj-$(CONFIG_IORT_TABLE) += iort.o +obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o # processor has its own "processor." module_param namespace processor-y := processor_driver.o diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c new file mode 100644 index 0000000..ccb19a0 --- /dev/null +++ b/drivers/acpi/spcr.c @@ -0,0 +1,85 @@ +/* + * Copyright (c) 2012, Intel Corporation + * Copyright (c) 2015, Red Hat, Inc. + * Copyright (c) 2015, 2016 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#define pr_fmt(fmt) "ACPI: SPCR: " fmt + +#include <linux/acpi.h> +#include <linux/console.h> +#include <linux/kernel.h> + +static struct acpi_table_spcr *spcr_table; + +int console_acpi_match(struct console *c, char **options) +{ + int err; + + if (!c->acpi_match) + return -ENODEV; + + if (!spcr_table) + return -EAGAIN; + + err = c->acpi_match(c, spcr_table); + if (err < 0) + return err; + + if (options) { + switch (spcr_table->baud_rate) { + case 3: + *options = "9600"; + break; + case 4: + *options = "19200"; + break; + case 6: + *options = "57600"; + break; + case 7: + *options = "115200"; + break; + default: + *options = ""; + break; + } + } + + return err; +} + +static int __init spcr_table_detect(void) +{ + struct acpi_table_header *table; + acpi_status status; + + if (acpi_disabled) + return -ENODEV; + + status = acpi_get_table(ACPI_SIG_SPCR, 0, &table); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get table, %s\n", msg); + return -EINVAL; + } + + if (table->revision < 2) + return -EOPNOTSUPP; + + spcr_table = (struct acpi_table_spcr *)table; + + pr_info("Console at 0x%016llx\n", spcr_table->serial_port.address); + + acpi_register_consoles_try_again(); + + return 0; +} + +arch_initcall(spcr_table_detect); diff --git a/include/linux/console.h b/include/linux/console.h index bd19434..94d0bd8 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -117,6 +117,7 @@ static inline int con_debug_leave(void) #define CON_BRL (32) /* Used for a braille device */ #define CON_EXTENDED (64) /* Use the extended output format a la /dev/kmsg */ +struct acpi_table_spcr; struct console { char name[16]; void (*write)(struct console *, const char *, unsigned); @@ -125,6 +126,7 @@ struct console { void (*unblank)(void); int (*setup)(struct console *, char *); int (*match)(struct console *, char *name, int idx, char *options); + int (*acpi_match)(struct console *, struct acpi_table_spcr *); short flags; short index; int cflag; @@ -132,6 +134,16 @@ struct console { struct console *next; }; +#ifdef CONFIG_ACPI +int console_acpi_match(struct console *c, char **options); +#else +static inline int console_acpi_match(struct console *c, char **options) +{ + return -ENODEV; +} +#endif +void acpi_register_consoles_try_again(void); + /* * for_each_console() allows you to iterate on each console */ diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 37e531f..3cf8cba 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2430,6 +2430,25 @@ static int __init keep_bootcon_setup(char *str) early_param("keep_bootcon", keep_bootcon_setup); +static DEFINE_MUTEX(acpi_consoles_delayed_mutex); +static struct console *acpi_consoles_delayed; + +void acpi_register_consoles_try_again(void) +{ + mutex_lock(&acpi_consoles_delayed_mutex); + while (acpi_consoles_delayed) { + + struct console *c = acpi_consoles_delayed; + + acpi_consoles_delayed = acpi_consoles_delayed->next; + + mutex_unlock(&acpi_consoles_delayed_mutex); + register_console(c); + mutex_lock(&acpi_consoles_delayed_mutex); + } + mutex_unlock(&acpi_consoles_delayed_mutex); +} + /* * The console driver calls this routine during kernel initialization * to register the console printing procedure with printk() and to @@ -2538,8 +2557,30 @@ void register_console(struct console *newcon) break; } - if (!(newcon->flags & CON_ENABLED)) - return; + if (!(newcon->flags & CON_ENABLED)) { + char *opts; + int err; + + if (newcon->index < 0) + newcon->index = 0; + + err = console_acpi_match(newcon, &opts); + + if (err == -EAGAIN) { + mutex_lock(&acpi_consoles_delayed_mutex); + newcon->next = acpi_consoles_delayed; + acpi_consoles_delayed = newcon; + mutex_unlock(&acpi_consoles_delayed_mutex); + return; + } else if (err < 0) { + return; + } else { + if (newcon->setup && newcon->setup(newcon, opts) != 0) + return; + newcon->flags |= CON_ENABLED | CON_CONSDEV; + preferred_console = true; + } + } /* * If we have a bootconsole, and are switching to a real console, @@ -2612,34 +2653,41 @@ void register_console(struct console *newcon) } EXPORT_SYMBOL(register_console); +static int delete_from_console_list(struct console **list, struct console *c) +{ + while (*list) { + struct console *cur = *list; + + if (cur == c) { + *list = cur->next; + return 0; + } + list = &cur->next; + } + return 1; +} + int unregister_console(struct console *console) { - struct console *a, *b; int res; pr_info("%sconsole [%s%d] disabled\n", (console->flags & CON_BOOT) ? "boot" : "" , console->name, console->index); + mutex_lock(&acpi_consoles_delayed_mutex); + res = delete_from_console_list(&acpi_consoles_delayed, console); + mutex_unlock(&acpi_consoles_delayed_mutex); + if (res == 0) + return res; + res = _braille_unregister_console(console); if (res) return res; - res = 1; console_lock(); - if (console_drivers == console) { - console_drivers=console->next; - res = 0; - } else if (console_drivers) { - for (a=console_drivers->next, b=console_drivers ; - a; b=a, a=b->next) { - if (a == console) { - b->next = a->next; - res = 0; - break; - } - } - } + + res = delete_from_console_list(&console_drivers, console); if (!res && (console->flags & CON_EXTENDED)) nr_ext_console_drivers--;
'ARM Server Base Boot Requiremets' [1] mention SPCR (Serial Port Console Redirection Table) [2] as a mandatory ACPI table that specifies the configuration of serial console. Parse this table and check if any registered console match the description. If it does, enable that console. To implement that, introduce a new member int (*acpi_match)(struct console *, struct acpi_table_spcr *) of struct console. It allows drivers to check if they provide a matching console device. [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> --- arch/arm64/Kconfig | 1 + drivers/acpi/Kconfig | 3 ++ drivers/acpi/Makefile | 1 + drivers/acpi/spcr.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/console.h | 12 +++++++ kernel/printk/printk.c | 82 +++++++++++++++++++++++++++++++++++++---------- 6 files changed, 167 insertions(+), 17 deletions(-) create mode 100644 drivers/acpi/spcr.c -- 2.7.0