'ARM Server Base Boot Requirements' [1] mentions SPCR (Serial Port
Console Redirection Table) [2] as a mandatory ACPI table that
specifies the configuration of serial console.
Move earlycon early_param handling to serial to parse earlycon option once
Patch "ACPI: add definitions of DBG2 subtypes" required for the next patch.
ACPI maintainers said that they had got it for the next release of ACPICA,
but it has not appeared in linux-next yet.
Parse SPCR table, setup earlycon and add register specified console.
Enable parsing this table on ARM64. Earlycon should be set up
as early as possible. ACPI boot tables are mapped in
arch/arm64/kernel/acpi.c:acpi_boot_table_init() called from setup_arch()
and that's where we parse spcr. So it has to be opted-in per-arch.
Implement console_match() for pl011.
Add EARLYCON_DECLARE to enable setting up earlycon for console specified in SPCR
Based on the work by Leif Lindholm [3]
Thanks to Peter Hurley for explaining how this should work.
Should be applied to next-20160322.
Tested on QEMU. SPCR support is included in QEMU's ARM mach-virt
since 2.4 release.
v5:
- drop patch "serial: pl011: use ACPI SPCR to setup 32-bit access" because
it is ugly. Also because Christopher Covington came with a better solution [4]
- remove error message when the table is not provided by ACPI (Andy Shevchenko)
- rewrite spcr.c following the suggestions by Peter Hurley
- add console_match() for pl011 in a separate patch
- add EARLYCON_DECLARE for pl011 in a separate patch
- patch "of/serial: move earlycon early_param handling to serial" was added from
the GDB2 series
v4:
https://lkml.kernel.org/g/[email protected]
- drop patch "ACPI: change __init to __ref for early_acpi_os_unmap_memory()"
ACPI developers work on a new API and asked not to do that.
Instead, use acpi_get_table_with_size()/early_acpi_os_unmap_memory() once
and cache the result. (Lv Zheng)
- fix some style issues (Yury Norov)
v3:
https://lkml.kernel.org/g/[email protected]
Greg Kroah-Hartman did not like v2 so I have rewritten this patchset:
- drop acpi_match() member of struct console
- drop implementations of this member for pl011 and 8250
- drop the patch that renames some vars in printk.c as it is not needed anymore
- drop patch that introduces system wide acpi_table_parse2().
Instead introduce a custom acpi_table_parse_spcr() in spcr.c
Instead of introducing a new match_acpi() member of struct console,
this patchset introduces a new function acpi_console_check().
This function is called when a new uart is registered at serial_core.c
the same way OF code checks for console. If the registered uart is the
console specified by SPCR table, this function calls add_preferred_console()
The restrictions of this approach are:
- only serial consoles can be set up
- only consoles specified by the memory/io address can be set up
(SPCR can specify devices by PCI id/PCI address)
v2:
https://lkml.kernel.org/g/[email protected]
- don't use SPCR if user specified console in command line
- fix initialization order of newcon->index = 0
- rename some variables at printk.c (Joe Perches, Peter Hurley)
- enable ACPI_SPCR_TABLE in a separate patch (Andy Shevchenko)
- remove the retry loop for console registering (Peter Hurley).
Instead, obtain SPCR with acpi_get_table(). That works after
call to acpi_early_init() i. e. in any *_initcall()
- describe design decision behind introducing acpi_match() (Peter Hurley)
- fix compilation for x86 + ACPI (Graeme Gregory)
- introduce DBG2 constants in a separate patch (Andy Shevchenko)
- fix a typo in DBG2 constants (Andy Shevchenko)
- add ACPI_DBG2_ARM_SBSA_32BIT constant (Christopher Covington)
- add support for ACPI_DBG2_ARM_SBSA_* consoles (Christopher Covington)
- add documentation for functions
- add a patch that uses SPCR to find if SBSA serial driver should use 32-bit
accessor functions (Christopher Covington)
- change __init to __ref for early_acpi_os_unmap_memory() in a separate patch
- introduce acpi_table_parse2() in a separate patch
- fix fetching the SPCR table early (Mark Salter)
- add a patch from Mark Salter that introduces support for matching 8250-based
consoles
v1:
https://lkml.kernel.org/g/[email protected]
[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
[3] https://lkml.kernel.org/g/[email protected]
[4] https://lkml.kernel.org/g/[email protected]
Aleksey Makarov (5):
ACPI: add definitions of DBG2 subtypes
ACPI: parse SPCR and enable matching console
ACPI: enable ACPI_SPCR_TABLE on ARM64
serial: pl011: add console matching function
serial: pl011: add EARLYCON_DECLARE
Leif Lindholm (1):
of/serial: move earlycon early_param handling to serial
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/acpi.c | 2 +
drivers/acpi/Kconfig | 3 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/spcr.c | 102 ++++++++++++++++++++++++++++++++++++++++
drivers/of/fdt.c | 11 +----
drivers/tty/serial/amba-pl011.c | 57 ++++++++++++++++++++++
drivers/tty/serial/earlycon.c | 12 +++--
include/acpi/actbl2.h | 5 ++
include/linux/acpi.h | 8 ++++
include/linux/of_fdt.h | 2 +
11 files changed, 191 insertions(+), 13 deletions(-)
create mode 100644 drivers/acpi/spcr.c
--
2.7.4
From: Leif Lindholm <[email protected]>
We have multiple "earlycon" early_param handlers - merge the DT one into
the main earlycon one. It's a cleanup that also will be useful
to decide if ACPI SPCR earlycon should be set up.
Signed-off-by: Leif Lindholm <[email protected]>
Signed-off-by: Aleksey Makarov <[email protected]>
---
drivers/of/fdt.c | 11 +----------
drivers/tty/serial/earlycon.c | 3 ++-
include/linux/of_fdt.h | 2 ++
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 3349d2a..0547256 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -805,7 +805,7 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
#ifdef CONFIG_SERIAL_EARLYCON
-static int __init early_init_dt_scan_chosen_serial(void)
+int __init early_init_dt_scan_chosen_serial(void)
{
int offset;
const char *p, *q, *options = NULL;
@@ -849,15 +849,6 @@ static int __init early_init_dt_scan_chosen_serial(void)
}
return -ENODEV;
}
-
-static int __init setup_of_earlycon(char *buf)
-{
- if (buf)
- return 0;
-
- return early_init_dt_scan_chosen_serial();
-}
-early_param("earlycon", setup_of_earlycon);
#endif
/**
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 067783f..d217366 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/io.h>
+#include <linux/of_fdt.h>
#include <linux/serial_core.h>
#include <linux/sizes.h>
#include <linux/of.h>
@@ -209,7 +210,7 @@ static int __init param_setup_earlycon(char *buf)
* don't generate a warning from parse_early_params() in that case
*/
if (!buf || !buf[0])
- return 0;
+ return early_init_dt_scan_chosen_serial();
err = setup_earlycon(buf);
if (err == -ENOENT || err == -EALREADY)
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 2fbe868..56b2a43 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -63,6 +63,7 @@ extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
int depth, void *data);
extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
int depth, void *data);
+extern int early_init_dt_scan_chosen_serial(void);
extern void early_init_fdt_scan_reserved_mem(void);
extern void early_init_fdt_reserve_self(void);
extern void early_init_dt_add_memory_arch(u64 base, u64 size);
@@ -91,6 +92,7 @@ extern void early_get_first_memblock_info(void *, phys_addr_t *);
extern u64 of_flat_dt_translate_address(unsigned long node);
extern void of_fdt_limit_memory(int limit);
#else /* CONFIG_OF_FLATTREE */
+static inline int early_init_dt_scan_chosen_serial(void) { return -ENODEV; }
static inline void early_init_fdt_scan_reserved_mem(void) {}
static inline void early_init_fdt_reserve_self(void) {}
static inline const char *of_flat_dt_get_machine_name(void) { return NULL; }
--
2.7.4
This patch adds definition of early console data for pl011.
It allows setting up an early console with a string passed in
command line or compiled by the ACPI SPCR table handler.
Signed-off-by: Aleksey Makarov <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 59b743f..af6f87cf 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2385,6 +2385,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
return 0;
}
OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
+EARLYCON_DECLARE(pl011, pl011_early_console_setup);
#else
#define AMBA_CONSOLE NULL
--
2.7.4
This patch adds function pl011_console_match() that implements
method match of struct console. It allows to match consoles against
data specified in a string, for example taken from command line or
compiled by ACPI SPCR table handler.
Signed-off-by: Aleksey Makarov <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 56 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7c198e0..59b743f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2287,12 +2287,68 @@ static int __init pl011_console_setup(struct console *co, char *options)
return uart_set_options(&uap->port, co, baud, parity, bits, flow);
}
+/**
+ * pl011_console_match - non-standard console matching
+ * @co: registering console
+ * @name: name from console command line
+ * @idx: index from console command line
+ * @options: ptr to option string from console command line
+ *
+ * Only attempts to match console command lines of the form:
+ * console=pl011,mmio|mmio32,<addr>[,<options>]
+ * console=pl011,0x<addr>[,<options>]
+ * This form is used to register an initial earlycon boot console and
+ * replace it with the amba_console at pl011 driver init.
+ *
+ * Performs console setup for a match (as required by interface)
+ * If no <options> are specified, then assume the h/w is already setup.
+ *
+ * Returns 0 if console matches; otherwise non-zero to use default matching
+ */
+static int __init pl011_console_match(struct console *co, char *name, int idx,
+ char *options)
+{
+ char match[] = "pl011"; /* pl011-specific earlycon name */
+ unsigned char iotype;
+ unsigned long addr;
+ int i;
+
+ if (strncmp(name, match, 5) != 0)
+ return -ENODEV;
+
+ if (uart_parse_earlycon(options, &iotype, &addr, &options))
+ return -ENODEV;
+
+ /* try to match the port specified on the command line */
+ for (i = 0; i < ARRAY_SIZE(amba_ports); i++) {
+ struct uart_port *port;
+
+ if (amba_ports[i] == NULL)
+ continue;
+
+ port = &amba_ports[i]->port;
+
+ if (port->iotype != iotype)
+ continue;
+ if ((iotype == UPIO_MEM || iotype == UPIO_MEM32)
+ && (port->mapbase != addr))
+ continue;
+
+ co->index = i;
+ port->cons = co;
+ return pl011_console_setup(co, options);
+ }
+
+ return -ENODEV;
+}
+
static struct uart_driver amba_reg;
static struct console amba_console = {
.name = "ttyAMA",
.write = pl011_console_write,
.device = uart_console_device,
.setup = pl011_console_setup,
+ .match = pl011_console_match,
.flags = CON_PRINTBUFFER,
.index = -1,
.data = &amba_reg,
--
2.7.4
SBBR mentions SPCR as a mandatory ACPI table. So enable it for ARM64
Earlycon should be set up as early as possible. ACPI boot tables are
mapped in arch/arm64/kernel/acpi.c:acpi_boot_table_init() called from
setup_arch() and that's where we parse SPCR. So it has to be opted-in
per-arch.
Signed-off-by: Aleksey Makarov <[email protected]>
---
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/acpi.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07b6a16..c77b4b2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -3,6 +3,7 @@ config ARM64
select ACPI_CCA_REQUIRED if ACPI
select ACPI_GENERIC_GSI if ACPI
select ACPI_REDUCED_HARDWARE_ONLY if ACPI
+ select ACPI_SPCR_TABLE if ACPI
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index d1ce8e2..36d6f70 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -208,6 +208,8 @@ void __init acpi_boot_table_init(void)
pr_err("Failed to init ACPI tables\n");
if (!param_acpi_force)
disable_acpi();
+ } else {
+ parse_spcr();
}
}
--
2.7.4
'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 <[email protected]>
---
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;
+}
+
+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*/
--
2.7.4
The recent version of Microsoft Debug Port Table 2 (DBG2) [1]
specifies additional serial debug port subtypes. These constants
are also referred by Serial Port Console Redirection Table (SPCR) [2]
Add these constants.
[1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
Signed-off-by: Aleksey Makarov <[email protected]>
---
include/acpi/actbl2.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index a4ef625..652f747 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -371,6 +371,11 @@ struct acpi_dbg2_device {
#define ACPI_DBG2_16550_COMPATIBLE 0x0000
#define ACPI_DBG2_16550_SUBSET 0x0001
+#define ACPI_DBG2_ARM_PL011 0x0003
+#define ACPI_DBG2_ARM_SBSA_32BIT 0x000D
+#define ACPI_DBG2_ARM_SBSA_GENERIC 0x000E
+#define ACPI_DBG2_ARM_DCC 0x000F
+#define ACPI_DBG2_BCM2835 0x0010
#define ACPI_DBG2_1394_STANDARD 0x0000
--
2.7.4
On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov
<[email protected]> 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)
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 22, 2016 at 5:46 AM, Aleksey Makarov
<[email protected]> wrote:
> From: Leif Lindholm <[email protected]>
>
> We have multiple "earlycon" early_param handlers - merge the DT one into
> the main earlycon one. It's a cleanup that also will be useful
> to decide if ACPI SPCR earlycon should be set up.
How so? Isn't that determined by whether we have ACPI tables or a DT?
This goes against trying to limit places of FDT parsing and keeping
the early scanning all within fdt.c. At least it is not in the arch
code. That said, I'm okay with this change if it helps.
Rob
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 <[email protected]>
> ---
> 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
Hi Leif,
[auto build test ERROR on next-20160322]
[also build test ERROR on v4.5]
[cannot apply to pm/linux-next v4.5-rc7 v4.5-rc6 v4.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Aleksey-Makarov/ACPI-parse-the-SPCR-table/20160322-185247
config: microblaze-mmu_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=microblaze
All errors (new ones prefixed by >>):
>> arch/microblaze/kernel/prom.c:48:19: error: conflicting types for 'early_init_dt_scan_chosen_serial'
static int __init early_init_dt_scan_chosen_serial(unsigned long node,
^
In file included from arch/microblaze/kernel/prom.c:33:0:
include/linux/of_fdt.h:66:12: note: previous declaration of 'early_init_dt_scan_chosen_serial' was here
extern int early_init_dt_scan_chosen_serial(void);
^
vim +/early_init_dt_scan_chosen_serial +48 arch/microblaze/kernel/prom.c
12e84142 Michal Simek 2009-03-27 42 #include <asm/sections.h>
12e84142 Michal Simek 2009-03-27 43 #include <asm/pci-bridge.h>
12e84142 Michal Simek 2009-03-27 44
12e84142 Michal Simek 2009-03-27 45 #ifdef CONFIG_EARLY_PRINTK
9d0c4dfe Rob Herring 2014-04-01 46 static const char *stdout;
2aa8e375 Michal Simek 2011-04-14 47
c0d997fb Michal Simek 2012-12-13 @48 static int __init early_init_dt_scan_chosen_serial(unsigned long node,
12e84142 Michal Simek 2009-03-27 49 const char *uname, int depth, void *data)
12e84142 Michal Simek 2009-03-27 50 {
9d0c4dfe Rob Herring 2014-04-01 51 int l;
:::::: The code at line 48 was first introduced by commit
:::::: c0d997fb4c4f202c55a4ed8ab9b714a81a16e5ac microblaze: Add static qualifiers
:::::: TO: Michal Simek <[email protected]>
:::::: CC: Michal Simek <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <[email protected]>
>> ---
>> 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
> <[email protected]> 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 03/22/2016 03:46 AM, Aleksey Makarov wrote:
> This patch adds definition of early console data for pl011.
> It allows setting up an early console with a string passed in
> command line or compiled by the ACPI SPCR table handler.
This shouldn't be necessary in 4.5+ since OF_EARLYCON_DECLARE()
should be equivalent to EARLYCON_DECLARE() as well.
Regards,
Peter Hurley
> Signed-off-by: Aleksey Makarov <[email protected]>
> ---
> drivers/tty/serial/amba-pl011.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 59b743f..af6f87cf 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2385,6 +2385,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
> return 0;
> }
> OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
> +EARLYCON_DECLARE(pl011, pl011_early_console_setup);
>
> #else
> #define AMBA_CONSOLE NULL
>
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 02:15 PM, Rob Herring wrote:
> On Tue, Mar 22, 2016 at 5:46 AM, Aleksey Makarov
> <[email protected]> wrote:
>> From: Leif Lindholm <[email protected]>
>>
>> We have multiple "earlycon" early_param handlers - merge the DT one into
>> the main earlycon one. It's a cleanup that also will be useful
>> to decide if ACPI SPCR earlycon should be set up.
>
> How so? Isn't that determined by whether we have ACPI tables or a DT?
Oops, I missed this. This patch was not only to do parsing in one place,
but also to allow addition of code that decide which of ACPI/DT should be used
for earlycon initialization based on acpi_disabled.
I will fix this, thank you.
> This goes against trying to limit places of FDT parsing and keeping
> the early scanning all within fdt.c. At least it is not in the arch
> code. That said, I'm okay with this change if it helps.
>
> Rob
>
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
>> <[email protected]> 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 07:15 PM, Peter Hurley wrote:
> On 03/22/2016 03:46 AM, Aleksey Makarov wrote:
>> This patch adds definition of early console data for pl011.
>> It allows setting up an early console with a string passed in
>> command line or compiled by the ACPI SPCR table handler.
>
> This shouldn't be necessary in 4.5+ since OF_EARLYCON_DECLARE()
> should be equivalent to EARLYCON_DECLARE() as well.
Ok, I will test it and drop this patch
Thank you
Aleksey Makarov
>
> Regards,
> Peter Hurley
>
>
>> Signed-off-by: Aleksey Makarov <[email protected]>
>> ---
>> drivers/tty/serial/amba-pl011.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 59b743f..af6f87cf 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -2385,6 +2385,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
>> return 0;
>> }
>> OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
>> +EARLYCON_DECLARE(pl011, pl011_early_console_setup);
>>
>> #else
>> #define AMBA_CONSOLE NULL
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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
>>> <[email protected]> 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
On 03/22/2016 10:09 AM, Aleksey Makarov wrote:
>
> On 03/22/2016 07:15 PM, Peter Hurley wrote:
>> On 03/22/2016 03:46 AM, Aleksey Makarov wrote:
>>> This patch adds definition of early console data for pl011.
>>> It allows setting up an early console with a string passed in
>>> command line or compiled by the ACPI SPCR table handler.
>>
>> This shouldn't be necessary in 4.5+ since OF_EARLYCON_DECLARE()
>> should be equivalent to EARLYCON_DECLARE() as well.
Sorry, I wrote 4.5+ but I meant 4.6+
> Ok, I will test it and drop this patch
>
> Thank you
> Aleksey Makarov
>
>>
>> Regards,
>> Peter Hurley
>>
>>
>>> Signed-off-by: Aleksey Makarov <[email protected]>
>>> ---
>>> drivers/tty/serial/amba-pl011.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>>> index 59b743f..af6f87cf 100644
>>> --- a/drivers/tty/serial/amba-pl011.c
>>> +++ b/drivers/tty/serial/amba-pl011.c
>>> @@ -2385,6 +2385,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
>>> return 0;
>>> }
>>> OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
>>> +EARLYCON_DECLARE(pl011, pl011_early_console_setup);
>>>
>>> #else
>>> #define AMBA_CONSOLE NULL
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>