Hi Yury, Thank you for review/suggestions. I will address it all in the next version except for... On 02/21/2016 12:42 PM, Yury Norov wrote: > On Mon, Feb 15, 2016 at 09:05:26PM +0300, Aleksey Makarov wrote: >> 'ARM Server Base Boot Requiremets' [1] mentions 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. >> >> Introduce a new function acpi_console_check(). At the uart port >> registration, this function checks if the ACPI SPCR table specifies >> its argument of type struct uart_port to be a console >> and if so calls add_preferred_console(). >> >> [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@xxxxxxxxxx> >> --- >> drivers/acpi/Kconfig | 3 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/spcr.c | 97 ++++++++++++++++++++++++++++++++++++++++ >> drivers/tty/serial/serial_core.c | 14 +++++- >> include/linux/acpi.h | 10 +++++ >> 5 files changed, 123 insertions(+), 2 deletions(-) >> create mode 100644 drivers/acpi/spcr.c >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 65fb483..5611eb6 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER >> >> endif >> >> +config ACPI_SPCR_TABLE >> + bool >> + >> config ACPI_SLEEP >> bool >> depends on SUSPEND || HIBERNATION >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 346101c..708b143 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o >> obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o >> obj-$(CONFIG_ACPI_BGRT) += bgrt.o >> obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o >> +obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o >> obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o >> >> # processor has its own "processor." module_param namespace >> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c >> new file mode 100644 >> index 0000000..a1eca91 >> --- /dev/null >> +++ b/drivers/acpi/spcr.c >> @@ -0,0 +1,97 @@ >> +/* >> + * 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> >> +#include <linux/serial_core.h> >> + >> +static int acpi_table_parse_spcr(int (*handler)(struct acpi_table_spcr *table, >> + void *data), void *data) >> +{ >> + struct acpi_table_spcr *table = NULL; > Hi Alexey, > > Few minor comments... > > Are you sure you need thin initialization? > >> + acpi_size table_size; >> + acpi_status status; >> + int err; >> + >> + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, >> + (struct acpi_table_header **)&table, >> + &table_size); >> + >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + err = handler(table, data); >> + >> + early_acpi_os_unmap_memory(table, table_size); >> + >> + return err; >> +} >> + >> +static int spcr_table_handler_check(struct acpi_table_spcr *table, void *data) > > The name of function is not clear for me. > You're not only checking here, but also > adding console. > >> +{ >> + struct uart_port *uport = data; >> + char *options; >> + >> + if (table->header.revision < 2) >> + return -EOPNOTSUPP; >> + >> + switch (table->baud_rate) { > > You don't need 'options' if your big condition returns false, > so you can evaluate it inside conditional block. > >> + case 3: >> + options = "9600"; >> + break; >> + case 4: >> + options = "19200"; >> + break; >> + case 6: >> + options = "57600"; >> + break; >> + case 7: >> + options = "115200"; >> + break; >> + default: >> + options = ""; >> + break; > > 'break' isnot needed here ... except for this. No, it is not, but C language style guides tell us to have it here. Also see the first example from Documentation/CodingStyle where they have it. Thank you Aleksey Makarov > >> + } >> + >> + if ((table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY && > > Nitpick: just for better readability, I'd split it onto a set of > separated checks: > > foo() { > if (!cond1) > return 0; > > if (!cond2) > return 0; > ... > > switch () { > case 1: > ... > case 2: > ... > case 3: > ... > default: > ... > } > > return 1; > } > >> + table->serial_port.address == (u64)uport->mapbase) || >> + (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO && >> + table->serial_port.address == (u64)uport->iobase)) { >> + pr_info("adding preferred console [%s%d]\n", uport->cons->name, >> + uport->line); >> + add_preferred_console(uport->cons->name, uport->line, options); >> + return 1; >> + >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * acpi_console_check - Check if uart matches the console specified by SPCR. >> + * >> + * @uport: uart port to check >> + * >> + * This function checks if the ACPI SPCR table specifies @uport to be a console >> + * and if so calls add_preferred_console() >> + * >> + * Return: a non-error value if the console matches. >> + */ >> +bool acpi_console_check(struct uart_port *uport) >> +{ >> + if (acpi_disabled || console_set_on_cmdline) >> + return false; >> + >> + return acpi_table_parse_spcr(spcr_table_handler_check, uport) > 0; >> +} >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index a126a60..459ab54 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -34,6 +34,7 @@ >> #include <linux/serial_core.h> >> #include <linux/delay.h> >> #include <linux/mutex.h> >> +#include <linux/acpi.h> >> >> #include <asm/irq.h> >> #include <asm/uaccess.h> >> @@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport) >> spin_lock_init(&uport->lock); >> lockdep_set_class(&uport->lock, &port_lock_key); >> } >> - if (uport->cons && uport->dev) >> - of_console_check(uport->dev->of_node, uport->cons->name, uport->line); >> + >> + /* >> + * Support both open FW and ACPI access to console definitions. >> + * Both of_console_check() and acpi_console_check() will call >> + * add_preferred_console() if a console definition is found. >> + */ >> + if (uport->cons && uport->dev) { >> + if (!acpi_console_check(uport)) >> + of_console_check(uport->dev->of_node, uport->cons->name, >> + uport->line); >> + } >> >> uart_configure_port(drv, state, uport); >> >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 06ed7e5..ea0c297 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev, >> #define acpi_probe_device_table(t) ({ int __r = 0; __r;}) >> #endif >> >> +struct uart_port; >> +#ifdef CONFIG_ACPI_SPCR_TABLE >> +bool acpi_console_check(struct uart_port *uport); >> +#else >> +static inline bool acpi_console_check(struct uart_port *uport) >> +{ >> + return FALSE; >> +} >> +#endif >> + >> #endif /*_LINUX_ACPI_H*/ >> -- >> 2.7.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
