| Message ID | 1458643595-14719-4-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov <aleksey.makarov@linaro.org> 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, setup earlycon and enable the specified console. > > Thanks to Peter Hurley for explaining how this should work. > > [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html > [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > > +++ b/drivers/acpi/spcr.c > @@ -0,0 +1,102 @@ > +/* > + * 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 bool init_earlycon; > + > +void __init init_spcr_earlycon(void) > +{ > + init_earlycon = true; > +} > + > +int __init parse_spcr(void) > +{ > + static char opts[64]; > + struct acpi_table_spcr *table; > + acpi_size table_size; > + acpi_status status; > + char *uart; > + char *iotype; > + int baud_rate; > + int err = 0; > + > + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, > + (struct acpi_table_header **)&table, > + &table_size); > + > + if (ACPI_FAILURE(status)) > + return -ENOENT; > + > + if (table->header.revision < 2) { > + err = -EINVAL; > + pr_err("wrong table version\n"); > + goto done; > + } > + > + iotype = (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? > + "mmio" : "io"; > + > + switch (table->interface_type) { > + case ACPI_DBG2_ARM_SBSA_32BIT: > + iotype = "mmio32"; > + /* fall through */ > + case ACPI_DBG2_ARM_PL011: > + case ACPI_DBG2_ARM_SBSA_GENERIC: > + case ACPI_DBG2_BCM2835: > + uart = "pl011"; > + break; > + case ACPI_DBG2_16550_COMPATIBLE: > + case ACPI_DBG2_16550_SUBSET: > + uart = "uart"; > + break; > + default: > + err = -ENOENT; > + goto done; > + } > + > + switch (table->baud_rate) { > + case 3: > + baud_rate = 9600; > + break; > + case 4: > + baud_rate = 19200; > + break; > + case 6: > + baud_rate = 57600; > + break; > + case 7: > + baud_rate = 115200; > + break; > + default: > + err = -ENOENT; > + goto done; > + } > + > + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, > + table->serial_port.address, baud_rate); You may use snprintf(), though my question here what would happen on 32-bit kernel when you supply 64-bit address as an option? > + > + pr_info("console: %s", opts); > + > + if (init_earlycon) > + setup_earlycon(opts); > + > + err = add_preferred_console(uart, 0, opts + strlen(uart) + 1); > + > +done: > + early_acpi_os_unmap_memory((void __iomem *)table, table_size); > + return err; > +} > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > index d217366..ec030c9 100644 > --- a/drivers/tty/serial/earlycon.c > +++ b/drivers/tty/serial/earlycon.c > @@ -22,6 +22,7 @@ > #include <linux/sizes.h> > #include <linux/of.h> > #include <linux/of_fdt.h> > +#include <linux/acpi.h> > > #ifdef CONFIG_FIX_EARLYCON_MEM > #include <asm/fixmap.h> > @@ -206,11 +207,15 @@ static int __init param_setup_earlycon(char *buf) > int err; > > /* > - * Just 'earlycon' is a valid param for devicetree earlycons; > - * don't generate a warning from parse_early_params() in that case > + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR > + * earlycons; don't generate a warning from parse_early_params() > + * in that case > */ > - if (!buf || !buf[0]) > - return early_init_dt_scan_chosen_serial(); > + if (!buf || !buf[0]) { > + init_spcr_earlycon(); > + early_init_dt_scan_chosen_serial(); > + return 0; And you hide an error? > + } > > err = setup_earlycon(buf); > if (err == -ENOENT || err == -EALREADY)
On Tue, Mar 22, 2016 at 01:46:30PM +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, setup earlycon and enable the specified console. > > Thanks to Peter Hurley for explaining how this should work. > > [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html > [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx > > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > --- > drivers/acpi/Kconfig | 3 ++ > drivers/acpi/Makefile | 1 + > drivers/acpi/spcr.c | 102 ++++++++++++++++++++++++++++++++++++++++++ > drivers/tty/serial/earlycon.c | 13 ++++-- > include/linux/acpi.h | 8 ++++ > 5 files changed, 123 insertions(+), 4 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 7395928..f70ae14 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -82,6 +82,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..1ec82f9 > --- /dev/null > +++ b/drivers/acpi/spcr.c > @@ -0,0 +1,102 @@ > +/* > + * 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 bool init_earlycon; > + > +void __init init_spcr_earlycon(void) > +{ > + init_earlycon = true; > +} > + 1. I see you keep in mind multiple access. Then you'd worry about race conditions as well. In this case, I'd consider atomic access to variable. 2. It seems you need is_init() helper too. > +int __init parse_spcr(void) > +{ > + static char opts[64]; > + struct acpi_table_spcr *table; > + acpi_size table_size; > + acpi_status status; > + char *uart; > + char *iotype; > + int baud_rate; > + int err = 0; You can do not initialize 'err'. > + > + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, > + (struct acpi_table_header **)&table, > + &table_size); > + > + if (ACPI_FAILURE(status)) > + return -ENOENT; > + > + if (table->header.revision < 2) { > + err = -EINVAL; > + pr_err("wrong table version\n"); > + goto done; > + } > + > + iotype = (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? > + "mmio" : "io"; > + > + switch (table->interface_type) { > + case ACPI_DBG2_ARM_SBSA_32BIT: > + iotype = "mmio32"; > + /* fall through */ > + case ACPI_DBG2_ARM_PL011: > + case ACPI_DBG2_ARM_SBSA_GENERIC: > + case ACPI_DBG2_BCM2835: > + uart = "pl011"; > + break; > + case ACPI_DBG2_16550_COMPATIBLE: > + case ACPI_DBG2_16550_SUBSET: > + uart = "uart"; > + break; > + default: > + err = -ENOENT; > + goto done; > + } > + > + switch (table->baud_rate) { > + case 3: > + baud_rate = 9600; > + break; > + case 4: > + baud_rate = 19200; > + break; > + case 6: > + baud_rate = 57600; > + break; > + case 7: > + baud_rate = 115200; > + break; > + default: > + err = -ENOENT; > + goto done; > + } > + > + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, > + table->serial_port.address, baud_rate); As 'opts' is static, don't you need to take some measures to prevent concurrent access. The simpler measure for me is to declare it on stack. > + > + pr_info("console: %s", opts); > + > + if (init_earlycon) > + setup_earlycon(opts); > + > + err = add_preferred_console(uart, 0, opts + strlen(uart) + 1); > + > +done: > + early_acpi_os_unmap_memory((void __iomem *)table, table_size); > + return err; > +} > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > index d217366..ec030c9 100644 > --- a/drivers/tty/serial/earlycon.c > +++ b/drivers/tty/serial/earlycon.c > @@ -22,6 +22,7 @@ > #include <linux/sizes.h> > #include <linux/of.h> > #include <linux/of_fdt.h> > +#include <linux/acpi.h> > > #ifdef CONFIG_FIX_EARLYCON_MEM > #include <asm/fixmap.h> > @@ -206,11 +207,15 @@ static int __init param_setup_earlycon(char *buf) > int err; > > /* > - * Just 'earlycon' is a valid param for devicetree earlycons; > - * don't generate a warning from parse_early_params() in that case > + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR > + * earlycons; don't generate a warning from parse_early_params() > + * in that case > */ > - if (!buf || !buf[0]) > - return early_init_dt_scan_chosen_serial(); > + if (!buf || !buf[0]) { > + init_spcr_earlycon(); > + early_init_dt_scan_chosen_serial(); > + return 0; > + } > > err = setup_earlycon(buf); > if (err == -ENOENT || err == -EALREADY) > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 06ed7e5..ebbb212 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1004,4 +1004,12 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev, > #define acpi_probe_device_table(t) ({ int __r = 0; __r;}) > #endif > > +#ifdef CONFIG_ACPI_SPCR_TABLE > +int parse_spcr(void); > +void init_spcr_earlycon(void); > +#else > +static inline int parse_spcr(void) { return 0; } > +static inline void init_spcr_earlycon(void) {} > +#endif > + > #endif /*_LINUX_ACPI_H*/ > -- > 2.7.4
On 03/22/2016 05:26 AM, Yury Norov wrote: > On Tue, Mar 22, 2016 at 01:46:30PM +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, setup earlycon and enable the specified console. >> >> Thanks to Peter Hurley for explaining how this should work. >> >> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html >> [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx >> >> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> >> --- >> drivers/acpi/Kconfig | 3 ++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/spcr.c | 102 ++++++++++++++++++++++++++++++++++++++++++ >> drivers/tty/serial/earlycon.c | 13 ++++-- >> include/linux/acpi.h | 8 ++++ >> 5 files changed, 123 insertions(+), 4 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 7395928..f70ae14 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -82,6 +82,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..1ec82f9 >> --- /dev/null >> +++ b/drivers/acpi/spcr.c >> @@ -0,0 +1,102 @@ >> +/* >> + * 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 bool init_earlycon; >> + >> +void __init init_spcr_earlycon(void) >> +{ >> + init_earlycon = true; >> +} >> + > > 1. I see you keep in mind multiple access. Concurrent access is not a concern here: only the boot cpu is running and intrs are off. The "init_earlycon" flag is used because parsing the "earlycon" early param is earlier than parsing ACPI tables. Then you'd worry about race > conditions as well. In this case, I'd consider atomic access to > variable. > 2. It seems you need is_init() helper too. > >> +int __init parse_spcr(void) >> +{ >> + static char opts[64]; >> + struct acpi_table_spcr *table; >> + acpi_size table_size; >> + acpi_status status; >> + char *uart; >> + char *iotype; >> + int baud_rate; >> + int err = 0; > > You can do not initialize 'err'. Why? > >> + >> + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, >> + (struct acpi_table_header **)&table, >> + &table_size); >> + >> + if (ACPI_FAILURE(status)) >> + return -ENOENT; >> + >> + if (table->header.revision < 2) { >> + err = -EINVAL; >> + pr_err("wrong table version\n"); >> + goto done; >> + } >> + >> + iotype = (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? >> + "mmio" : "io"; >> + >> + switch (table->interface_type) { >> + case ACPI_DBG2_ARM_SBSA_32BIT: >> + iotype = "mmio32"; >> + /* fall through */ >> + case ACPI_DBG2_ARM_PL011: >> + case ACPI_DBG2_ARM_SBSA_GENERIC: >> + case ACPI_DBG2_BCM2835: >> + uart = "pl011"; >> + break; >> + case ACPI_DBG2_16550_COMPATIBLE: >> + case ACPI_DBG2_16550_SUBSET: >> + uart = "uart"; >> + break; >> + default: >> + err = -ENOENT; >> + goto done; >> + } >> + >> + switch (table->baud_rate) { >> + case 3: >> + baud_rate = 9600; >> + break; >> + case 4: >> + baud_rate = 19200; >> + break; >> + case 6: >> + baud_rate = 57600; >> + break; >> + case 7: >> + baud_rate = 115200; >> + break; >> + default: >> + err = -ENOENT; >> + goto done; >> + } >> + >> + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, >> + table->serial_port.address, baud_rate); > > As 'opts' is static, don't you need to take some measures to prevent > concurrent access. The simpler measure for me is to declare it on > stack. Again, concurrent access is not a concern at this early time. Regards, Peter Hurley >> + >> + pr_info("console: %s", opts); >> + >> + if (init_earlycon) >> + setup_earlycon(opts); >> + >> + err = add_preferred_console(uart, 0, opts + strlen(uart) + 1); >> + >> +done: >> + early_acpi_os_unmap_memory((void __iomem *)table, table_size); >> + return err; >> +} >> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c >> index d217366..ec030c9 100644 >> --- a/drivers/tty/serial/earlycon.c >> +++ b/drivers/tty/serial/earlycon.c >> @@ -22,6 +22,7 @@ >> #include <linux/sizes.h> >> #include <linux/of.h> >> #include <linux/of_fdt.h> >> +#include <linux/acpi.h> >> >> #ifdef CONFIG_FIX_EARLYCON_MEM >> #include <asm/fixmap.h> >> @@ -206,11 +207,15 @@ static int __init param_setup_earlycon(char *buf) >> int err; >> >> /* >> - * Just 'earlycon' is a valid param for devicetree earlycons; >> - * don't generate a warning from parse_early_params() in that case >> + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR >> + * earlycons; don't generate a warning from parse_early_params() >> + * in that case >> */ >> - if (!buf || !buf[0]) >> - return early_init_dt_scan_chosen_serial(); >> + if (!buf || !buf[0]) { >> + init_spcr_earlycon(); >> + early_init_dt_scan_chosen_serial(); >> + return 0; >> + } >> >> err = setup_earlycon(buf); >> if (err == -ENOENT || err == -EALREADY) >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 06ed7e5..ebbb212 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -1004,4 +1004,12 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev, >> #define acpi_probe_device_table(t) ({ int __r = 0; __r;}) >> #endif >> >> +#ifdef CONFIG_ACPI_SPCR_TABLE >> +int parse_spcr(void); >> +void init_spcr_earlycon(void); >> +#else >> +static inline int parse_spcr(void) { return 0; } >> +static inline void init_spcr_earlycon(void) {} >> +#endif >> + >> #endif /*_LINUX_ACPI_H*/ >> -- >> 2.7.4
On 03/22/2016 04:09 AM, Andy Shevchenko wrote: > On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov > <aleksey.makarov@linaro.org> 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, setup earlycon and enable the specified console. >> >> Thanks to Peter Hurley for explaining how this should work. >> >> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html >> [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx >> > >> +++ b/drivers/acpi/spcr.c >> @@ -0,0 +1,102 @@ >> +/* >> + * 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 bool init_earlycon; >> + >> +void __init init_spcr_earlycon(void) >> +{ >> + init_earlycon = true; >> +} >> + >> +int __init parse_spcr(void) >> +{ >> + static char opts[64]; >> + struct acpi_table_spcr *table; >> + acpi_size table_size; >> + acpi_status status; >> + char *uart; >> + char *iotype; >> + int baud_rate; >> + int err = 0; >> + >> + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, >> + (struct acpi_table_header **)&table, >> + &table_size); >> + >> + if (ACPI_FAILURE(status)) >> + return -ENOENT; >> + >> + if (table->header.revision < 2) { >> + err = -EINVAL; >> + pr_err("wrong table version\n"); >> + goto done; >> + } >> + >> + iotype = (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? >> + "mmio" : "io"; >> + >> + switch (table->interface_type) { >> + case ACPI_DBG2_ARM_SBSA_32BIT: >> + iotype = "mmio32"; >> + /* fall through */ >> + case ACPI_DBG2_ARM_PL011: >> + case ACPI_DBG2_ARM_SBSA_GENERIC: >> + case ACPI_DBG2_BCM2835: >> + uart = "pl011"; >> + break; >> + case ACPI_DBG2_16550_COMPATIBLE: >> + case ACPI_DBG2_16550_SUBSET: >> + uart = "uart"; >> + break; >> + default: >> + err = -ENOENT; >> + goto done; >> + } >> + >> + switch (table->baud_rate) { >> + case 3: >> + baud_rate = 9600; >> + break; >> + case 4: >> + baud_rate = 19200; >> + break; >> + case 6: >> + baud_rate = 57600; >> + break; >> + case 7: >> + baud_rate = 115200; >> + break; >> + default: >> + err = -ENOENT; >> + goto done; >> + } >> + >> + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, >> + table->serial_port.address, baud_rate); > > You may use snprintf(), though my question here what would happen on > 32-bit kernel when you supply 64-bit address as an option? Yeah this should probably use %pa for the printf specifier. But note this exposes underlying bug in the earlycon support, because that was originally written without 32/64-mixed bitness in mind; ie., the address is parsed and handled as unsigned long in most places. >> + >> + pr_info("console: %s", opts); >> + >> + if (init_earlycon) >> + setup_earlycon(opts); >> + >> + err = add_preferred_console(uart, 0, opts + strlen(uart) + 1); >> + >> +done: >> + early_acpi_os_unmap_memory((void __iomem *)table, table_size); >> + return err; >> +} >> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c >> index d217366..ec030c9 100644 >> --- a/drivers/tty/serial/earlycon.c >> +++ b/drivers/tty/serial/earlycon.c >> @@ -22,6 +22,7 @@ >> #include <linux/sizes.h> >> #include <linux/of.h> >> #include <linux/of_fdt.h> >> +#include <linux/acpi.h> >> >> #ifdef CONFIG_FIX_EARLYCON_MEM >> #include <asm/fixmap.h> >> @@ -206,11 +207,15 @@ static int __init param_setup_earlycon(char *buf) >> int err; >> >> /* >> - * Just 'earlycon' is a valid param for devicetree earlycons; >> - * don't generate a warning from parse_early_params() in that case >> + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR >> + * earlycons; don't generate a warning from parse_early_params() >> + * in that case >> */ >> - if (!buf || !buf[0]) >> - return early_init_dt_scan_chosen_serial(); >> + if (!buf || !buf[0]) { >> + init_spcr_earlycon(); > >> + early_init_dt_scan_chosen_serial(); >> + return 0; > > And you hide an error? Well, this is a little bit tricky because "earlycon" early parameter with missing /chosen/stdout-path node is no longer an error, since ACPI may be specifying the earlycon instead. Regards, Peter Hurley >> + } >> >> err = setup_earlycon(buf); >> if (err == -ENOENT || err == -EALREADY) >
On Tue, Mar 22, 2016 at 07:57:04AM -0700, Peter Hurley wrote: [...] > >> +static bool init_earlycon; > >> + > >> +void __init init_spcr_earlycon(void) > >> +{ > >> + init_earlycon = true; > >> +} > >> + > > > > 1. I see you keep in mind multiple access. > > Concurrent access is not a concern here: only the boot cpu is running > and intrs are off. > > The "init_earlycon" flag is used because parsing the "earlycon" early param > is earlier than parsing ACPI tables. > OK got it. My concern is that it's generic code, and parse_spcr() is public function. I think corresponding comment is needed at least. The other option is to make it race-safe and forget. I prefer second one, moreover it's 2 simple changes. > > Then you'd worry about race > > conditions as well. In this case, I'd consider atomic access to > > variable. > > 2. It seems you need is_init() helper too. > > > >> +int __init parse_spcr(void) > >> +{ > >> + static char opts[64]; > >> + struct acpi_table_spcr *table; > >> + acpi_size table_size; > >> + acpi_status status; > >> + char *uart; > >> + char *iotype; > >> + int baud_rate; > >> + int err = 0; > > > > You can do not initialize 'err'. > > Why? > Because there's no path here that doesn't init err with some value. So this initialization is useless waste of cycles. [...]
On 03/22/2016 07:07 PM, Peter Hurley wrote: > On 03/22/2016 04:09 AM, Andy Shevchenko wrote: >> On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov >> <aleksey.makarov@linaro.org> wrote: >>> + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, >>> + table->serial_port.address, baud_rate); >> >> You may use snprintf(), though my question here what would happen on >> 32-bit kernel when you supply 64-bit address as an option? > > Yeah this should probably use %pa for the printf specifier. > > But note this exposes underlying bug in the earlycon support, because > that was originally written without 32/64-mixed bitness in mind; ie., > the address is parsed and handled as unsigned long in most places. I don't quite follow this. table->serial_port.address is explicitly u64, not pointer, so, according to printk-formats.txt %llx is ok here, %pa is wrong. Am I missing something? >>> /* >>> - * Just 'earlycon' is a valid param for devicetree earlycons; >>> - * don't generate a warning from parse_early_params() in that case >>> + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR >>> + * earlycons; don't generate a warning from parse_early_params() >>> + * in that case >>> */ >>> - if (!buf || !buf[0]) >>> - return early_init_dt_scan_chosen_serial(); >>> + if (!buf || !buf[0]) { >>> + init_spcr_earlycon(); >> >>> + early_init_dt_scan_chosen_serial(); >>> + return 0; >> >> And you hide an error? > > Well, this is a little bit tricky because "earlycon" early parameter with > missing /chosen/stdout-path node is no longer an error, since ACPI may be > specifying the earlycon instead. Agree, but note the email by Rob Herring. The code should be like this: if (!buf || !buf[0]) { if (acpi_disabled) { return early_init_dt_scan_chosen_serial(); } else { init_spcr_earlycon(); return 0; } } But that requires to have made ACPI/DT decision at this point. Thank you Aleksey
On 03/22/2016 07:51 PM, Yury Norov wrote: > On Tue, Mar 22, 2016 at 07:57:04AM -0700, Peter Hurley wrote: > > [...] > >>>> +static bool init_earlycon; >>>> + >>>> +void __init init_spcr_earlycon(void) >>>> +{ >>>> + init_earlycon = true; >>>> +} >>>> + >>> >>> 1. I see you keep in mind multiple access. >> >> Concurrent access is not a concern here: only the boot cpu is running >> and intrs are off. >> >> The "init_earlycon" flag is used because parsing the "earlycon" early param >> is earlier than parsing ACPI tables. >> > > OK got it. My concern is that it's generic code, and parse_spcr() is public > function. I think corresponding comment is needed at least. The other option is > to make it race-safe and forget. I prefer second one, moreover it's 2 simple > changes. I would not call this function public. It is made non-static only to allow callin it from another compilation unit. It should be called only once at initializatioin time. I will add a comment about this, thank you. >> Then you'd worry about race >>> conditions as well. In this case, I'd consider atomic access to >>> variable. >>> 2. It seems you need is_init() helper too. >>> >>>> +int __init parse_spcr(void) >>>> +{ >>>> + static char opts[64]; >>>> + struct acpi_table_spcr *table; >>>> + acpi_size table_size; >>>> + acpi_status status; >>>> + char *uart; >>>> + char *iotype; >>>> + int baud_rate; >>>> + int err = 0; >>> >>> You can do not initialize 'err'. >> >> Why? >> > > Because there's no path here that doesn't init err with some value. > So this initialization is useless waste of cycles. > > [...] I agree, will fix this. Thank you Aleksey
On 03/22/2016 10:04 AM, Aleksey Makarov wrote: > > > On 03/22/2016 07:07 PM, Peter Hurley wrote: >> On 03/22/2016 04:09 AM, Andy Shevchenko wrote: >>> On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov >>> <aleksey.makarov@linaro.org> wrote: > >>>> + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, >>>> + table->serial_port.address, baud_rate); >>> >>> You may use snprintf(), though my question here what would happen on >>> 32-bit kernel when you supply 64-bit address as an option? >> >> Yeah this should probably use %pa for the printf specifier. >> >> But note this exposes underlying bug in the earlycon support, because >> that was originally written without 32/64-mixed bitness in mind; ie., >> the address is parsed and handled as unsigned long in most places. > > I don't quite follow this. table->serial_port.address is explicitly u64, > not pointer, so, according to printk-formats.txt %llx is ok here, %pa is wrong. > Am I missing something? No, you're right. There is still the underlying bug I noted; I'll fix that in this release cycle. >>>> /* >>>> - * Just 'earlycon' is a valid param for devicetree earlycons; >>>> - * don't generate a warning from parse_early_params() in that case >>>> + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR >>>> + * earlycons; don't generate a warning from parse_early_params() >>>> + * in that case >>>> */ >>>> - if (!buf || !buf[0]) >>>> - return early_init_dt_scan_chosen_serial(); >>>> + if (!buf || !buf[0]) { >>>> + init_spcr_earlycon(); >>> >>>> + early_init_dt_scan_chosen_serial(); >>>> + return 0; >>> >>> And you hide an error? >> >> Well, this is a little bit tricky because "earlycon" early parameter with >> missing /chosen/stdout-path node is no longer an error, since ACPI may be >> specifying the earlycon instead. > > Agree, but note the email by Rob Herring. The code should be like this: > > if (!buf || !buf[0]) { > if (acpi_disabled) { > return early_init_dt_scan_chosen_serial(); > } else { > init_spcr_earlycon(); > return 0; > } > } Ok. > But that requires to have made ACPI/DT decision at this point. It's an unfortunate command-line option dependency, but I think it's ok for the moment. The same problem would exist if DT could be disabled via the command line. > > Thank you > Aleksey >
On 03/22/2016 09:51 AM, Yury Norov wrote: > On Tue, Mar 22, 2016 at 07:57:04AM -0700, Peter Hurley wrote: > > [...] > >>>> +static bool init_earlycon; >>>> + >>>> +void __init init_spcr_earlycon(void) >>>> +{ >>>> + init_earlycon = true; >>>> +} >>>> + >>> >>> 1. I see you keep in mind multiple access. >> >> Concurrent access is not a concern here: only the boot cpu is running >> and intrs are off. >> >> The "init_earlycon" flag is used because parsing the "earlycon" early param >> is earlier than parsing ACPI tables. >> > > OK got it. My concern is that it's generic code, and parse_spcr() is public > function. I think corresponding comment is needed at least. The other option is > to make it race-safe and forget. I prefer second one, moreover it's 2 simple > changes. Earlycon is generic only in the sense of platform independence, not in the sense of temporal independence. There is no serialization in earlycon, anywhere. Adding serialization will unnecessarily confuse casual observers into believing it is necessary. An argument could be made that earlycon needs some standalone documentation, but I don't think this patch needs to be that. >> Then you'd worry about race >>> conditions as well. In this case, I'd consider atomic access to >>> variable. >>> 2. It seems you need is_init() helper too. >>> >>>> +int __init parse_spcr(void) >>>> +{ >>>> + static char opts[64]; >>>> + struct acpi_table_spcr *table; >>>> + acpi_size table_size; >>>> + acpi_status status; >>>> + char *uart; >>>> + char *iotype; >>>> + int baud_rate; >>>> + int err = 0; >>> >>> You can do not initialize 'err'. >> >> Why? >> > > Because there's no path here that doesn't init err with some value. > So this initialization is useless waste of cycles. Ok. Regards, Peter Hurley
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 7395928..f70ae14 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -82,6 +82,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..1ec82f9 --- /dev/null +++ b/drivers/acpi/spcr.c @@ -0,0 +1,102 @@ +/* + * 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 bool init_earlycon; + +void __init init_spcr_earlycon(void) +{ + init_earlycon = true; +} + +int __init parse_spcr(void) +{ + static char opts[64]; + struct acpi_table_spcr *table; + acpi_size table_size; + acpi_status status; + char *uart; + char *iotype; + int baud_rate; + int err = 0; + + status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0, + (struct acpi_table_header **)&table, + &table_size); + + if (ACPI_FAILURE(status)) + return -ENOENT; + + if (table->header.revision < 2) { + err = -EINVAL; + pr_err("wrong table version\n"); + goto done; + } + + iotype = (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? + "mmio" : "io"; + + switch (table->interface_type) { + case ACPI_DBG2_ARM_SBSA_32BIT: + iotype = "mmio32"; + /* fall through */ + case ACPI_DBG2_ARM_PL011: + case ACPI_DBG2_ARM_SBSA_GENERIC: + case ACPI_DBG2_BCM2835: + uart = "pl011"; + break; + case ACPI_DBG2_16550_COMPATIBLE: + case ACPI_DBG2_16550_SUBSET: + uart = "uart"; + break; + default: + err = -ENOENT; + goto done; + } + + switch (table->baud_rate) { + case 3: + baud_rate = 9600; + break; + case 4: + baud_rate = 19200; + break; + case 6: + baud_rate = 57600; + break; + case 7: + baud_rate = 115200; + break; + default: + err = -ENOENT; + goto done; + } + + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype, + table->serial_port.address, baud_rate); + + pr_info("console: %s", opts); + + if (init_earlycon) + setup_earlycon(opts); + + err = add_preferred_console(uart, 0, opts + strlen(uart) + 1); + +done: + early_acpi_os_unmap_memory((void __iomem *)table, table_size); + return err; +} diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c index d217366..ec030c9 100644 --- a/drivers/tty/serial/earlycon.c +++ b/drivers/tty/serial/earlycon.c @@ -22,6 +22,7 @@ #include <linux/sizes.h> #include <linux/of.h> #include <linux/of_fdt.h> +#include <linux/acpi.h> #ifdef CONFIG_FIX_EARLYCON_MEM #include <asm/fixmap.h> @@ -206,11 +207,15 @@ static int __init param_setup_earlycon(char *buf) int err; /* - * Just 'earlycon' is a valid param for devicetree earlycons; - * don't generate a warning from parse_early_params() in that case + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR + * earlycons; don't generate a warning from parse_early_params() + * in that case */ - if (!buf || !buf[0]) - return early_init_dt_scan_chosen_serial(); + if (!buf || !buf[0]) { + init_spcr_earlycon(); + early_init_dt_scan_chosen_serial(); + return 0; + } err = setup_earlycon(buf); if (err == -ENOENT || err == -EALREADY) diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 06ed7e5..ebbb212 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1004,4 +1004,12 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev, #define acpi_probe_device_table(t) ({ int __r = 0; __r;}) #endif +#ifdef CONFIG_ACPI_SPCR_TABLE +int parse_spcr(void); +void init_spcr_earlycon(void); +#else +static inline int parse_spcr(void) { return 0; } +static inline void init_spcr_earlycon(void) {} +#endif + #endif /*_LINUX_ACPI_H*/
'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, setup earlycon and enable the specified console. Thanks to Peter Hurley for explaining how this should work. [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html [2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> --- drivers/acpi/Kconfig | 3 ++ drivers/acpi/Makefile | 1 + drivers/acpi/spcr.c | 102 ++++++++++++++++++++++++++++++++++++++++++ drivers/tty/serial/earlycon.c | 13 ++++-- include/linux/acpi.h | 8 ++++ 5 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 drivers/acpi/spcr.c