| Message ID | 1441716217-23786-4-git-send-email-leif.lindholm@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 08, 2015 at 01:43:35PM +0100, Leif Lindholm wrote: > The ACPI DBG2 table defines a debug console. Add support for parsing it > and using it to select earlycon destination when no arguments provided. > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> [...] > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c > new file mode 100644 > index 0000000..a985890 > --- /dev/null > +++ b/drivers/acpi/console.c > @@ -0,0 +1,103 @@ > +/* > + * Copyright (c) 2012, Intel Corporation > + * Copyright (c) 2015, 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 DEBUG Why? > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/kernel.h> > +#include <linux/serial_core.h> > + > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) Use ARRAY_SIZE (from kernel.h). > + > +#ifdef CONFIG_SERIAL_EARLYCON > +static int use_earlycon __initdata; > +static int __init setup_acpi_earlycon(char *buf) > +{ > + if (!buf) > + use_earlycon = 1; > + > + return 0; > +} > +early_param("earlycon", setup_acpi_earlycon); It seems a shame to add this after folding the OF case into the earlycon code. What necessitates this being a separate early_param? Why is it too early to parse DBG2? [...] > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > index 2bda09a..c063cbb 100644 > --- a/drivers/tty/serial/earlycon.c > +++ b/drivers/tty/serial/earlycon.c > @@ -13,6 +13,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <linux/acpi.h> > #include <linux/console.h> > #include <linux/kernel.h> > #include <linux/init.h> > @@ -184,12 +185,16 @@ 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 or ACPI earlycons; > + * ACPI cannot be parsed yet, so return without action if enabled. > + * Otherwise, attempt initialization using DT. > */ > - if (!buf || !buf[0]) > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > + if (!buf || !buf[0]) { > + if (!acpi_disabled) > + return 0; > + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) > return early_init_dt_scan_chosen_serial(); > + } It would be much nicer if we could handle the ACPI earlycon parsing in the same place. As above, I assume I'm missing something that prevents that. Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-09-08 at 13:43 +0100, Leif Lindholm wrote: > The ACPI DBG2 table defines a debug console. Add support for parsing it > and using it to select earlycon destination when no arguments provided. > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > arch/arm64/kernel/acpi.c | 2 + > drivers/acpi/Makefile | 1 + > drivers/acpi/console.c | 103 ++++++++++++++++++++++++++++++++++++++++++ > drivers/of/fdt.c | 2 +- > drivers/tty/serial/earlycon.c | 16 ++++--- > include/linux/acpi.h | 4 ++ > include/linux/serial_core.h | 9 ++-- > 7 files changed, 126 insertions(+), 11 deletions(-) > create mode 100644 drivers/acpi/console.c > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index b9a5623..be7600a 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void) > if (!param_acpi_force) > disable_acpi(); > } > + > + acpi_early_console_probe(); > } > > void __init acpi_gic_init(void) > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index b5e7cd8..a89587d 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT > # > obj-y += tables.o > obj-$(CONFIG_X86) += blacklist.o > +obj-y += console.o obj-$(CONFIG_SERIAL_EARLYCON) += console.o to eliminate whole-file #ifdef > > # > # ACPI Core Subsystem (Interpreter) > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c > new file mode 100644 > index 0000000..a985890 > --- /dev/null > +++ b/drivers/acpi/console.c > @@ -0,0 +1,103 @@ > +/* > + * Copyright (c) 2012, Intel Corporation > + * Copyright (c) 2015, 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 DEBUG > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/kernel.h> > +#include <linux/serial_core.h> > + > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) > + > +#ifdef CONFIG_SERIAL_EARLYCON > +static int use_earlycon __initdata; > +static int __init setup_acpi_earlycon(char *buf) > +{ > + if (!buf) > + use_earlycon = 1; > + > + return 0; > +} > +early_param("earlycon", setup_acpi_earlycon); > + > +extern struct earlycon_id __earlycon_table[]; > + > +static __initdata struct { > + int id; > + const char *name; > +} subtypes[] = { > + {0, "uart8250"}, > + {1, "uart8250"}, > + {2, NULL}, > + {3, "pl011"}, > +}; Instead of having a table here, why not have an ACPI_EARLYCON_DECLARE() where individual drivers can provide an id similar to OF_EARLYCON_DECLARE() providing compatible strings? > + > +static int __init acpi_setup_earlycon(unsigned long addr, const char *driver) > +{ > + const struct earlycon_id *match; > + > + for (match = __earlycon_table; match->name[0]; match++) > + if (strcmp(driver, match->name) == 0) > + return setup_earlycon_driver(addr, match->setup); > + > + return -ENODEV; > +} > + > +static int __init acpi_parse_dbg2(struct acpi_table_header *table) > +{ > + struct acpi_table_dbg2 *dbg2; > + struct acpi_dbg2_device *entry; > + void *tbl_end; > + > + dbg2 = (struct acpi_table_dbg2 *)table; > + if (!dbg2) { > + pr_debug("DBG2 not present.\n"); > + return -ENODEV; > + } > + > + tbl_end = (void *)table + table->length; > + > + entry = (struct acpi_dbg2_device *)((void *)dbg2 + dbg2->info_offset); > + > + while (((void *)entry) + sizeof(struct acpi_dbg2_device) < tbl_end) { > + struct acpi_generic_address *addr; > + > + if (entry->revision != 0) { > + pr_debug("DBG2 revision %d not supported.\n", > + entry->revision); > + return -ENODEV; > + } > + > + addr = (void *)entry + entry->base_address_offset; > + > + pr_debug("DBG2 PROBE - console (%04x:%04x).\n", > + entry->port_type, entry->port_subtype); > + > + if (use_earlycon && > + (entry->port_type == ACPI_DBG2_SERIAL_PORT) && > + (entry->port_subtype < NUM_ELEMS(subtypes))) > + acpi_setup_earlycon(addr->address, > + subtypes[entry->port_subtype].name); Don't we need to handle space_id (and bit width) as well as address? > + > + entry = (struct acpi_dbg2_device *) > + ((void *)entry + entry->length); > + } > + > + return 0; > +} > + > +int __init acpi_early_console_probe(void) > +{ > + acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2); > + > + return 0; > +} > +#endif /* CONFIG_SERIAL_EARLYCON */ > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index fcfc4c7..a96209f 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -829,7 +829,7 @@ int __init early_init_dt_scan_chosen_serial(void) > if (!addr) > return -ENXIO; > > - of_setup_earlycon(addr, match->data); > + setup_earlycon_driver(addr, match->data); > return 0; > } > return -ENODEV; > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > index 2bda09a..c063cbb 100644 > --- a/drivers/tty/serial/earlycon.c > +++ b/drivers/tty/serial/earlycon.c > @@ -13,6 +13,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <linux/acpi.h> > #include <linux/console.h> > #include <linux/kernel.h> > #include <linux/init.h> > @@ -184,12 +185,16 @@ 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 or ACPI earlycons; > + * ACPI cannot be parsed yet, so return without action if enabled. > + * Otherwise, attempt initialization using DT. > */ > - if (!buf || !buf[0]) > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > + if (!buf || !buf[0]) { > + if (!acpi_disabled) How do we know for sure that "acpi" has been parsed before "earlycon"? > + return 0; > + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) > return early_init_dt_scan_chosen_serial(); > + } > > err = setup_earlycon(buf); > if (err == -ENOENT || err == -EALREADY) > @@ -198,8 +203,7 @@ static int __init param_setup_earlycon(char *buf) > } > early_param("earlycon", param_setup_earlycon); > > -int __init of_setup_earlycon(unsigned long addr, > - int (*setup)(struct earlycon_device *, const char *)) > +int __init setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup) > { > int err; > struct uart_port *port = &early_console_dev.port; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 7235c48..88cb9c1 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -811,4 +811,8 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev, > > #endif > > +#if defined(CONFIG_ACPI) && defined(CONFIG_SERIAL_EARLYCON) > +int __init acpi_early_console_probe(void); > +#endif > + > #endif /*_LINUX_ACPI_H*/ > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h > index 297d4fa..39e99b0 100644 > --- a/include/linux/serial_core.h > +++ b/include/linux/serial_core.h > @@ -339,14 +339,15 @@ struct earlycon_device { > unsigned int baud; > }; > > +typedef int (*earlycon_initfunc_t)(struct earlycon_device *, const char *); > + > struct earlycon_id { > - char name[16]; > - int (*setup)(struct earlycon_device *, const char *options); > + char name[16]; > + earlycon_initfunc_t setup; > } __aligned(32); > > extern int setup_earlycon(char *buf); > -extern int of_setup_earlycon(unsigned long addr, > - int (*setup)(struct earlycon_device *, const char *)); > +extern int setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup); > > #define EARLYCON_DECLARE(_name, func) \ > static const struct earlycon_id __earlycon_##_name \ -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 08, 2015 at 02:09:51PM +0100, Mark Rutland wrote: > On Tue, Sep 08, 2015 at 01:43:35PM +0100, Leif Lindholm wrote: > > The ACPI DBG2 table defines a debug console. Add support for parsing it > > and using it to select earlycon destination when no arguments provided. > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > [...] > > > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c > > new file mode 100644 > > index 0000000..a985890 > > --- /dev/null > > +++ b/drivers/acpi/console.c > > @@ -0,0 +1,103 @@ > > +/* > > + * Copyright (c) 2012, Intel Corporation > > + * Copyright (c) 2015, 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 DEBUG > > Why? Kept around from Lv Zheng's 2012 set. Will drop. > > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt > > + > > +#include <linux/acpi.h> > > +#include <linux/kernel.h> > > +#include <linux/serial_core.h> > > + > > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) > > Use ARRAY_SIZE (from kernel.h). I was sure there was something like that, but my grep-fu failed me. Will fix. > > + > > +#ifdef CONFIG_SERIAL_EARLYCON > > +static int use_earlycon __initdata; > > +static int __init setup_acpi_earlycon(char *buf) > > +{ > > + if (!buf) > > + use_earlycon = 1; > > + > > + return 0; > > +} > > +early_param("earlycon", setup_acpi_earlycon); > > It seems a shame to add this after folding the OF case into the earlycon > code. What necessitates this being a separate early_param? Why is it too > early to parse DBG2? Currently, we don't even know where our ACPI tables are at this point (efi_init() is called two functions after parse_early_param() in setup_arch). More specifically, because acpi_boot_table_init() is called even later than that. If we moved both of those earlier, we could drop the extra earlycon param handling for ACPI. That would of course reduce the ability to have dynamically configurable debug messages for both of these. > [...] > > > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > > index 2bda09a..c063cbb 100644 > > --- a/drivers/tty/serial/earlycon.c > > +++ b/drivers/tty/serial/earlycon.c > > @@ -13,6 +13,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > +#include <linux/acpi.h> > > #include <linux/console.h> > > #include <linux/kernel.h> > > #include <linux/init.h> > > @@ -184,12 +185,16 @@ 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 or ACPI earlycons; > > + * ACPI cannot be parsed yet, so return without action if enabled. > > + * Otherwise, attempt initialization using DT. > > */ > > - if (!buf || !buf[0]) > > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > + if (!buf || !buf[0]) { > > + if (!acpi_disabled) > > + return 0; > > + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > return early_init_dt_scan_chosen_serial(); > > + } > > It would be much nicer if we could handle the ACPI earlycon parsing in > the same place. As above, I assume I'm missing something that prevents > that. I don't disagree. / Leif -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 08, 2015 at 12:38:59PM -0400, Mark Salter wrote: > On Tue, 2015-09-08 at 13:43 +0100, Leif Lindholm wrote: > > The ACPI DBG2 table defines a debug console. Add support for parsing it > > and using it to select earlycon destination when no arguments provided. > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > --- > > arch/arm64/kernel/acpi.c | 2 + > > drivers/acpi/Makefile | 1 + > > drivers/acpi/console.c | 103 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/of/fdt.c | 2 +- > > drivers/tty/serial/earlycon.c | 16 ++++--- > > include/linux/acpi.h | 4 ++ > > include/linux/serial_core.h | 9 ++-- > > 7 files changed, 126 insertions(+), 11 deletions(-) > > create mode 100644 drivers/acpi/console.c > > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > > index b9a5623..be7600a 100644 > > --- a/arch/arm64/kernel/acpi.c > > +++ b/arch/arm64/kernel/acpi.c > > @@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void) > > if (!param_acpi_force) > > disable_acpi(); > > } > > + > > + acpi_early_console_probe(); > > } > > > > void __init acpi_gic_init(void) > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > index b5e7cd8..a89587d 100644 > > --- a/drivers/acpi/Makefile > > +++ b/drivers/acpi/Makefile > > @@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT > > # > > obj-y += tables.o > > obj-$(CONFIG_X86) += blacklist.o > > +obj-y += console.o > > obj-$(CONFIG_SERIAL_EARLYCON) += console.o > > to eliminate whole-file #ifdef Yes, that makes more sense for this patch standalone, but I felt it would be a bit weird to add the conditionality here only to delete it in the subsequent patch. I don't feel strongly about it. > > > > # > > # ACPI Core Subsystem (Interpreter) > > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c > > new file mode 100644 > > index 0000000..a985890 > > --- /dev/null > > +++ b/drivers/acpi/console.c > > @@ -0,0 +1,103 @@ > > +/* > > + * Copyright (c) 2012, Intel Corporation > > + * Copyright (c) 2015, 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 DEBUG > > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt > > + > > +#include <linux/acpi.h> > > +#include <linux/kernel.h> > > +#include <linux/serial_core.h> > > + > > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) > > + > > +#ifdef CONFIG_SERIAL_EARLYCON > > +static int use_earlycon __initdata; > > +static int __init setup_acpi_earlycon(char *buf) > > +{ > > + if (!buf) > > + use_earlycon = 1; > > + > > + return 0; > > +} > > +early_param("earlycon", setup_acpi_earlycon); > > + > > +extern struct earlycon_id __earlycon_table[]; > > + > > +static __initdata struct { > > + int id; > > + const char *name; > > +} subtypes[] = { > > + {0, "uart8250"}, > > + {1, "uart8250"}, > > + {2, NULL}, > > + {3, "pl011"}, > > +}; > > Instead of having a table here, why not have an ACPI_EARLYCON_DECLARE() > where individual drivers can provide an id similar to OF_EARLYCON_DECLARE() > providing compatible strings? The IDs are defined by the DBG2 specification, so it felt more natural to encapsulate it here. However, a comment to that effect would be useful. Or would you still prefer ACPI_EARLYCON_DECLARE(0, uart8250) ACPI_EARLYCON_DECLARE(1, uart8250) ... ? > > + > > +static int __init acpi_setup_earlycon(unsigned long addr, const char *driver) > > +{ > > + const struct earlycon_id *match; > > + > > + for (match = __earlycon_table; match->name[0]; match++) > > + if (strcmp(driver, match->name) == 0) > > + return setup_earlycon_driver(addr, match->setup); > > + > > + return -ENODEV; > > +} > > + > > +static int __init acpi_parse_dbg2(struct acpi_table_header *table) > > +{ > > + struct acpi_table_dbg2 *dbg2; > > + struct acpi_dbg2_device *entry; > > + void *tbl_end; > > + > > + dbg2 = (struct acpi_table_dbg2 *)table; > > + if (!dbg2) { > > + pr_debug("DBG2 not present.\n"); > > + return -ENODEV; > > + } > > + > > + tbl_end = (void *)table + table->length; > > + > > + entry = (struct acpi_dbg2_device *)((void *)dbg2 + dbg2->info_offset); > > + > > + while (((void *)entry) + sizeof(struct acpi_dbg2_device) < tbl_end) { > > + struct acpi_generic_address *addr; > > + > > + if (entry->revision != 0) { > > + pr_debug("DBG2 revision %d not supported.\n", > > + entry->revision); > > + return -ENODEV; > > + } > > + > > + addr = (void *)entry + entry->base_address_offset; > > + > > + pr_debug("DBG2 PROBE - console (%04x:%04x).\n", > > + entry->port_type, entry->port_subtype); > > + > > + if (use_earlycon && > > + (entry->port_type == ACPI_DBG2_SERIAL_PORT) && > > + (entry->port_subtype < NUM_ELEMS(subtypes))) > > + acpi_setup_earlycon(addr->address, > > + subtypes[entry->port_subtype].name); > > Don't we need to handle space_id (and bit width) as well as address? space_id? bit width? DBG2 is for quite primitive debug ports, already initialised by formware (or other pre-kernel agent). > > + > > + entry = (struct acpi_dbg2_device *) > > + ((void *)entry + entry->length); > > + } > > + > > + return 0; > > +} > > + > > +int __init acpi_early_console_probe(void) > > +{ > > + acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2); > > + > > + return 0; > > +} > > +#endif /* CONFIG_SERIAL_EARLYCON */ > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > index fcfc4c7..a96209f 100644 > > --- a/drivers/of/fdt.c > > +++ b/drivers/of/fdt.c > > @@ -829,7 +829,7 @@ int __init early_init_dt_scan_chosen_serial(void) > > if (!addr) > > return -ENXIO; > > > > - of_setup_earlycon(addr, match->data); > > + setup_earlycon_driver(addr, match->data); > > return 0; > > } > > return -ENODEV; > > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > > index 2bda09a..c063cbb 100644 > > --- a/drivers/tty/serial/earlycon.c > > +++ b/drivers/tty/serial/earlycon.c > > @@ -13,6 +13,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > +#include <linux/acpi.h> > > #include <linux/console.h> > > #include <linux/kernel.h> > > #include <linux/init.h> > > @@ -184,12 +185,16 @@ 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 or ACPI earlycons; > > + * ACPI cannot be parsed yet, so return without action if enabled. > > + * Otherwise, attempt initialization using DT. > > */ > > - if (!buf || !buf[0]) > > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > + if (!buf || !buf[0]) { > > + if (!acpi_disabled) > > How do we know for sure that "acpi" has been parsed before "earlycon"? Because "arch" comes before "drivers" in kernel image link order. *twitch* Yes, not the best argument ever. > > + return 0; > > + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > return early_init_dt_scan_chosen_serial(); > > + } > > > > err = setup_earlycon(buf); > > if (err == -ENOENT || err == -EALREADY) > > @@ -198,8 +203,7 @@ static int __init param_setup_earlycon(char *buf) > > } > > early_param("earlycon", param_setup_earlycon); > > > > -int __init of_setup_earlycon(unsigned long addr, > > - int (*setup)(struct earlycon_device *, const char *)) > > +int __init setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup) > > { > > int err; > > struct uart_port *port = &early_console_dev.port; > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index 7235c48..88cb9c1 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -811,4 +811,8 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev, > > > > #endif > > > > +#if defined(CONFIG_ACPI) && defined(CONFIG_SERIAL_EARLYCON) > > +int __init acpi_early_console_probe(void); > > +#endif > > + > > #endif /*_LINUX_ACPI_H*/ > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h > > index 297d4fa..39e99b0 100644 > > --- a/include/linux/serial_core.h > > +++ b/include/linux/serial_core.h > > @@ -339,14 +339,15 @@ struct earlycon_device { > > unsigned int baud; > > }; > > > > +typedef int (*earlycon_initfunc_t)(struct earlycon_device *, const char *); > > + > > struct earlycon_id { > > - char name[16]; > > - int (*setup)(struct earlycon_device *, const char *options); > > + char name[16]; > > + earlycon_initfunc_t setup; > > } __aligned(32); > > > > extern int setup_earlycon(char *buf); > > -extern int of_setup_earlycon(unsigned long addr, > > - int (*setup)(struct earlycon_device *, const char *)); > > +extern int setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup); > > > > #define EARLYCON_DECLARE(_name, func) \ > > static const struct earlycon_id __earlycon_##_name \ > -- 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/
On Tue, 2015-09-08 at 18:17 +0100, Leif Lindholm wrote: > On Tue, Sep 08, 2015 at 12:38:59PM -0400, Mark Salter wrote: > > On Tue, 2015-09-08 at 13:43 +0100, Leif Lindholm wrote: > > > The ACPI DBG2 table defines a debug console. Add support for parsing it > > > and using it to select earlycon destination when no arguments provided. > > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > > --- > > > arch/arm64/kernel/acpi.c | 2 + > > > drivers/acpi/Makefile | 1 + > > > drivers/acpi/console.c | 103 ++++++++++++++++++++++++++++++++++++++++++ > > > drivers/of/fdt.c | 2 +- > > > drivers/tty/serial/earlycon.c | 16 ++++--- > > > include/linux/acpi.h | 4 ++ > > > include/linux/serial_core.h | 9 ++-- > > > 7 files changed, 126 insertions(+), 11 deletions(-) > > > create mode 100644 drivers/acpi/console.c > > > > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > > > index b9a5623..be7600a 100644 > > > --- a/arch/arm64/kernel/acpi.c > > > +++ b/arch/arm64/kernel/acpi.c > > > @@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void) > > > if (!param_acpi_force) > > > disable_acpi(); > > > } > > > + > > > + acpi_early_console_probe(); > > > } > > > > > > void __init acpi_gic_init(void) > > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > > index b5e7cd8..a89587d 100644 > > > --- a/drivers/acpi/Makefile > > > +++ b/drivers/acpi/Makefile > > > @@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT > > > # > > > obj-y += tables.o > > > obj-$(CONFIG_X86) += blacklist.o > > > +obj-y += console.o > > > > obj-$(CONFIG_SERIAL_EARLYCON) += console.o > > > > to eliminate whole-file #ifdef > > Yes, that makes more sense for this patch standalone, but I felt it > would be a bit weird to add the conditionality here only to delete it > in the subsequent patch. I don't feel strongly about it. OIC. I didn't read ahead far enough. > > > > > > > # > > > # ACPI Core Subsystem (Interpreter) > > > diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c > > > new file mode 100644 > > > index 0000000..a985890 > > > --- /dev/null > > > +++ b/drivers/acpi/console.c > > > @@ -0,0 +1,103 @@ > > > +/* > > > + * Copyright (c) 2012, Intel Corporation > > > + * Copyright (c) 2015, 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 DEBUG > > > +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt > > > + > > > +#include <linux/acpi.h> > > > +#include <linux/kernel.h> > > > +#include <linux/serial_core.h> > > > + > > > +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) > > > + > > > +#ifdef CONFIG_SERIAL_EARLYCON > > > +static int use_earlycon __initdata; > > > +static int __init setup_acpi_earlycon(char *buf) > > > +{ > > > + if (!buf) > > > + use_earlycon = 1; > > > + > > > + return 0; > > > +} > > > +early_param("earlycon", setup_acpi_earlycon); > > > + > > > +extern struct earlycon_id __earlycon_table[]; > > > + > > > +static __initdata struct { > > > + int id; > > > + const char *name; > > > +} subtypes[] = { > > > + {0, "uart8250"}, > > > + {1, "uart8250"}, > > > + {2, NULL}, > > > + {3, "pl011"}, > > > +}; > > > > Instead of having a table here, why not have an ACPI_EARLYCON_DECLARE() > > where individual drivers can provide an id similar to OF_EARLYCON_DECLARE() > > providing compatible strings? > > The IDs are defined by the DBG2 specification, so it felt more > natural to encapsulate it here. However, a comment to that effect > would be useful. Or would you still prefer > ACPI_EARLYCON_DECLARE(0, uart8250) > ACPI_EARLYCON_DECLARE(1, uart8250) > ... > ? The idea is that the driver itself is only place that needs to handle a new uart type being supported rather than two places. > > > > + > > > +static int __init acpi_setup_earlycon(unsigned long addr, const char *driver) > > > +{ > > > + const struct earlycon_id *match; > > > + > > > + for (match = __earlycon_table; match->name[0]; match++) > > > + if (strcmp(driver, match->name) == 0) > > > + return setup_earlycon_driver(addr, match->setup); > > > + > > > + return -ENODEV; > > > +} > > > + > > > +static int __init acpi_parse_dbg2(struct acpi_table_header *table) > > > +{ > > > + struct acpi_table_dbg2 *dbg2; > > > + struct acpi_dbg2_device *entry; > > > + void *tbl_end; > > > + > > > + dbg2 = (struct acpi_table_dbg2 *)table; > > > + if (!dbg2) { > > > + pr_debug("DBG2 not present.\n"); > > > + return -ENODEV; > > > + } > > > + > > > + tbl_end = (void *)table + table->length; > > > + > > > + entry = (struct acpi_dbg2_device *)((void *)dbg2 + dbg2->info_offset); > > > + > > > + while (((void *)entry) + sizeof(struct acpi_dbg2_device) < tbl_end) { > > > + struct acpi_generic_address *addr; > > > + > > > + if (entry->revision != 0) { > > > + pr_debug("DBG2 revision %d not supported.\n", > > > + entry->revision); > > > + return -ENODEV; > > > + } > > > + > > > + addr = (void *)entry + entry->base_address_offset; > > > + > > > + pr_debug("DBG2 PROBE - console (%04x:%04x).\n", > > > + entry->port_type, entry->port_subtype); > > > + > > > + if (use_earlycon && > > > + (entry->port_type == ACPI_DBG2_SERIAL_PORT) && > > > + (entry->port_subtype < NUM_ELEMS(subtypes))) > > > + acpi_setup_earlycon(addr->address, > > > + subtypes[entry->port_subtype].name); > > > > Don't we need to handle space_id (and bit width) as well as address? > > space_id? bit width? > DBG2 is for quite primitive debug ports, already initialised by > formware (or other pre-kernel agent). How else would it work for x86 where uart may be mmio or ioport? Even with firmware initialization, kernel still needs to poke at registers. > > > > + > > > + entry = (struct acpi_dbg2_device *) > > > + ((void *)entry + entry->length); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +int __init acpi_early_console_probe(void) > > > +{ > > > + acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2); > > > + > > > + return 0; > > > +} > > > +#endif /* CONFIG_SERIAL_EARLYCON */ > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > > index fcfc4c7..a96209f 100644 > > > --- a/drivers/of/fdt.c > > > +++ b/drivers/of/fdt.c > > > @@ -829,7 +829,7 @@ int __init early_init_dt_scan_chosen_serial(void) > > > if (!addr) > > > return -ENXIO; > > > > > > - of_setup_earlycon(addr, match->data); > > > + setup_earlycon_driver(addr, match->data); > > > return 0; > > > } > > > return -ENODEV; > > > diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c > > > index 2bda09a..c063cbb 100644 > > > --- a/drivers/tty/serial/earlycon.c > > > +++ b/drivers/tty/serial/earlycon.c > > > @@ -13,6 +13,7 @@ > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > +#include <linux/acpi.h> > > > #include <linux/console.h> > > > #include <linux/kernel.h> > > > #include <linux/init.h> > > > @@ -184,12 +185,16 @@ 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 or ACPI earlycons; > > > + * ACPI cannot be parsed yet, so return without action if enabled. > > > + * Otherwise, attempt initialization using DT. > > > */ > > > - if (!buf || !buf[0]) > > > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > > + if (!buf || !buf[0]) { > > > + if (!acpi_disabled) > > > > How do we know for sure that "acpi" has been parsed before "earlycon"? > > Because "arch" comes before "drivers" in kernel image link order. > *twitch* > Yes, not the best argument ever. > > > > + return 0; > > > + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > > return early_init_dt_scan_chosen_serial(); > > > + } > > > > > > err = setup_earlycon(buf); > > > if (err == -ENOENT || err == -EALREADY) > > > @@ -198,8 +203,7 @@ static int __init param_setup_earlycon(char *buf) > > > } > > > early_param("earlycon", param_setup_earlycon); > > > > > > -int __init of_setup_earlycon(unsigned long addr, > > > - int (*setup)(struct earlycon_device *, const char *)) > > > +int __init setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup) > > > { > > > int err; > > > struct uart_port *port = &early_console_dev.port; > > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > > index 7235c48..88cb9c1 100644 > > > --- a/include/linux/acpi.h > > > +++ b/include/linux/acpi.h > > > @@ -811,4 +811,8 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev, > > > > > > #endif > > > > > > +#if defined(CONFIG_ACPI) && defined(CONFIG_SERIAL_EARLYCON) > > > +int __init acpi_early_console_probe(void); > > > +#endif > > > + > > > #endif /*_LINUX_ACPI_H*/ > > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h > > > index 297d4fa..39e99b0 100644 > > > --- a/include/linux/serial_core.h > > > +++ b/include/linux/serial_core.h > > > @@ -339,14 +339,15 @@ struct earlycon_device { > > > unsigned int baud; > > > }; > > > > > > +typedef int (*earlycon_initfunc_t)(struct earlycon_device *, const char *); > > > + > > > struct earlycon_id { > > > - char name[16]; > > > - int (*setup)(struct earlycon_device *, const char *options); > > > + char name[16]; > > > + earlycon_initfunc_t setup; > > > } __aligned(32); > > > > > > extern int setup_earlycon(char *buf); > > > -extern int of_setup_earlycon(unsigned long addr, > > > - int (*setup)(struct earlycon_device *, const char *)); > > > +extern int setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup); > > > > > > #define EARLYCON_DECLARE(_name, func) \ > > > static const struct earlycon_id __earlycon_##_name \ > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-09-08 at 18:17 +0100, Leif Lindholm wrote: > > > */ > > > - if (!buf || !buf[0]) > > > - if (IS_ENABLED(CONFIG_OF_FLATTREE)) > > > + if (!buf || !buf[0]) { > > > + if (!acpi_disabled) > > > > How do we know for sure that "acpi" has been parsed before "earlycon"? > > Because "arch" comes before "drivers" in kernel image link order. > *twitch* > Yes, not the best argument ever. > I don't think that matters. Things are parsed in order they are found on cmdline (except for early vs late distinction). -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > +#ifdef CONFIG_SERIAL_EARLYCON > > > +static int use_earlycon __initdata; > > > +static int __init setup_acpi_earlycon(char *buf) > > > +{ > > > + if (!buf) > > > + use_earlycon = 1; > > > + > > > + return 0; > > > +} > > > +early_param("earlycon", setup_acpi_earlycon); > > > > It seems a shame to add this after folding the OF case into the earlycon > > code. What necessitates this being a separate early_param? Why is it too > > early to parse DBG2? > > Currently, we don't even know where our ACPI tables are at this point > (efi_init() is called two functions after parse_early_param() in > setup_arch). More specifically, because acpi_boot_table_init() is > called even later than that. > > If we moved both of those earlier, we could drop the extra earlycon > param handling for ACPI. That would of course reduce the ability to > have dynamically configurable debug messages for both of these. Ok. Would you be able to put something in the commit message regarding the above, to make it clear why we need this multi-step dance (and why it's preferable to the alternative)? Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index b9a5623..be7600a 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -207,6 +207,8 @@ void __init acpi_boot_table_init(void) if (!param_acpi_force) disable_acpi(); } + + acpi_early_console_probe(); } void __init acpi_gic_init(void) diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index b5e7cd8..a89587d 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -10,6 +10,7 @@ ccflags-$(CONFIG_ACPI_DEBUG) += -DACPI_DEBUG_OUTPUT # obj-y += tables.o obj-$(CONFIG_X86) += blacklist.o +obj-y += console.o # # ACPI Core Subsystem (Interpreter) diff --git a/drivers/acpi/console.c b/drivers/acpi/console.c new file mode 100644 index 0000000..a985890 --- /dev/null +++ b/drivers/acpi/console.c @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2012, Intel Corporation + * Copyright (c) 2015, 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 DEBUG +#define pr_fmt(fmt) "ACPI: " KBUILD_MODNAME ": " fmt + +#include <linux/acpi.h> +#include <linux/kernel.h> +#include <linux/serial_core.h> + +#define NUM_ELEMS(x) (sizeof(x) / sizeof(*x)) + +#ifdef CONFIG_SERIAL_EARLYCON +static int use_earlycon __initdata; +static int __init setup_acpi_earlycon(char *buf) +{ + if (!buf) + use_earlycon = 1; + + return 0; +} +early_param("earlycon", setup_acpi_earlycon); + +extern struct earlycon_id __earlycon_table[]; + +static __initdata struct { + int id; + const char *name; +} subtypes[] = { + {0, "uart8250"}, + {1, "uart8250"}, + {2, NULL}, + {3, "pl011"}, +}; + +static int __init acpi_setup_earlycon(unsigned long addr, const char *driver) +{ + const struct earlycon_id *match; + + for (match = __earlycon_table; match->name[0]; match++) + if (strcmp(driver, match->name) == 0) + return setup_earlycon_driver(addr, match->setup); + + return -ENODEV; +} + +static int __init acpi_parse_dbg2(struct acpi_table_header *table) +{ + struct acpi_table_dbg2 *dbg2; + struct acpi_dbg2_device *entry; + void *tbl_end; + + dbg2 = (struct acpi_table_dbg2 *)table; + if (!dbg2) { + pr_debug("DBG2 not present.\n"); + return -ENODEV; + } + + tbl_end = (void *)table + table->length; + + entry = (struct acpi_dbg2_device *)((void *)dbg2 + dbg2->info_offset); + + while (((void *)entry) + sizeof(struct acpi_dbg2_device) < tbl_end) { + struct acpi_generic_address *addr; + + if (entry->revision != 0) { + pr_debug("DBG2 revision %d not supported.\n", + entry->revision); + return -ENODEV; + } + + addr = (void *)entry + entry->base_address_offset; + + pr_debug("DBG2 PROBE - console (%04x:%04x).\n", + entry->port_type, entry->port_subtype); + + if (use_earlycon && + (entry->port_type == ACPI_DBG2_SERIAL_PORT) && + (entry->port_subtype < NUM_ELEMS(subtypes))) + acpi_setup_earlycon(addr->address, + subtypes[entry->port_subtype].name); + + entry = (struct acpi_dbg2_device *) + ((void *)entry + entry->length); + } + + return 0; +} + +int __init acpi_early_console_probe(void) +{ + acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2); + + return 0; +} +#endif /* CONFIG_SERIAL_EARLYCON */ diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index fcfc4c7..a96209f 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -829,7 +829,7 @@ int __init early_init_dt_scan_chosen_serial(void) if (!addr) return -ENXIO; - of_setup_earlycon(addr, match->data); + setup_earlycon_driver(addr, match->data); return 0; } return -ENODEV; diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c index 2bda09a..c063cbb 100644 --- a/drivers/tty/serial/earlycon.c +++ b/drivers/tty/serial/earlycon.c @@ -13,6 +13,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/acpi.h> #include <linux/console.h> #include <linux/kernel.h> #include <linux/init.h> @@ -184,12 +185,16 @@ 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 or ACPI earlycons; + * ACPI cannot be parsed yet, so return without action if enabled. + * Otherwise, attempt initialization using DT. */ - if (!buf || !buf[0]) - if (IS_ENABLED(CONFIG_OF_FLATTREE)) + if (!buf || !buf[0]) { + if (!acpi_disabled) + return 0; + else if (IS_ENABLED(CONFIG_OF_FLATTREE)) return early_init_dt_scan_chosen_serial(); + } err = setup_earlycon(buf); if (err == -ENOENT || err == -EALREADY) @@ -198,8 +203,7 @@ static int __init param_setup_earlycon(char *buf) } early_param("earlycon", param_setup_earlycon); -int __init of_setup_earlycon(unsigned long addr, - int (*setup)(struct earlycon_device *, const char *)) +int __init setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup) { int err; struct uart_port *port = &early_console_dev.port; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 7235c48..88cb9c1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -811,4 +811,8 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev, #endif +#if defined(CONFIG_ACPI) && defined(CONFIG_SERIAL_EARLYCON) +int __init acpi_early_console_probe(void); +#endif + #endif /*_LINUX_ACPI_H*/ diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 297d4fa..39e99b0 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -339,14 +339,15 @@ struct earlycon_device { unsigned int baud; }; +typedef int (*earlycon_initfunc_t)(struct earlycon_device *, const char *); + struct earlycon_id { - char name[16]; - int (*setup)(struct earlycon_device *, const char *options); + char name[16]; + earlycon_initfunc_t setup; } __aligned(32); extern int setup_earlycon(char *buf); -extern int of_setup_earlycon(unsigned long addr, - int (*setup)(struct earlycon_device *, const char *)); +extern int setup_earlycon_driver(unsigned long addr, earlycon_initfunc_t setup); #define EARLYCON_DECLARE(_name, func) \ static const struct earlycon_id __earlycon_##_name \
The ACPI DBG2 table defines a debug console. Add support for parsing it and using it to select earlycon destination when no arguments provided. Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> --- arch/arm64/kernel/acpi.c | 2 + drivers/acpi/Makefile | 1 + drivers/acpi/console.c | 103 ++++++++++++++++++++++++++++++++++++++++++ drivers/of/fdt.c | 2 +- drivers/tty/serial/earlycon.c | 16 ++++--- include/linux/acpi.h | 4 ++ include/linux/serial_core.h | 9 ++-- 7 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 drivers/acpi/console.c