On 03/03/2016 08:48 PM, Peter Hurley wrote:

On 03/01/2016 08:57 AM, Aleksey Makarov wrote:

quoted

On 03/01/2016 06:53 PM, Peter Hurley wrote:

quoted

On 02/29/2016 04:42 AM, Aleksey Makarov wrote:

quoted

Add ACPI_DBG2_EARLYCON_DECLARE() macros that declares
an earlycon on the serial port specified in the DBG2 ACPI table.

Pass the string "earlycon=acpi_dbg2" to the kernel to activate it.

Callbacks for EARLYCON_DECLARE() and OF_EARLYCON_DECLARE()
can also be used for this macros.

Signed-off-by: Aleksey Makarov <redacted>
---
 Documentation/kernel-parameters.txt |  3 ++
 drivers/tty/serial/earlycon.c       | 60 +++++++++++++++++++++++++++++++++++++
 include/linux/acpi_dbg2.h           | 20 +++++++++++++
 3 files changed, 83 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e0a21e4..19b947b 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt

@@ -1072,6 +1072,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			A valid base address must be provided, and the serial
 			port must already be setup and configured.
 
+		acpi_dbg2
+			Use serial port specified by the DBG2 ACPI table.
+
 	earlyprintk=	[X86,SH,BLACKFIN,ARM,M68k]
 			earlyprintk=vga
 			earlyprintk=efi

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index d217366..9ba3a04 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>

@@ -200,6 +201,8 @@ int __init setup_earlycon(char *buf)
 	return -ENOENT;
 }
 
+static bool setup_dbg2_earlycon;
+
 /* early_param wrapper for setup_earlycon() */
 static int __init param_setup_earlycon(char *buf)
 {

@@ -212,6 +215,11 @@ static int __init param_setup_earlycon(char *buf)
 	if (!buf || !buf[0])
 		return early_init_dt_scan_chosen_serial();
 
+	if (!strcmp(buf, "acpi_dbg2")) {
+		setup_dbg2_earlycon = true;
+		return 0;
+	}

So this series doesn't start an ACPI earlycon at early_param time?
That doesn't seem very useful.

When does the ACPI earlycon actually start?
And don't say "when the DBG2 table is probed"; that much is obvious.

ACPI earlycon starts as soon as ACPI tables become accessible (setup_arch()).
I think that is still quite early.

I see now; the probe is in patch 6/7.

setup_arch()
  acpi_boot_table_init()
    acpi_probe_device_table()
      ...
        acpi_dbg2_setup()
          ->setup()
            acpi_setup_earlycon()

quoted

quoted

quoted

+
 	err = setup_earlycon(buf);
 	if (err == -ENOENT || err == -EALREADY)
 		return 0;

@@ -286,3 +294,55 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
 }
 
 #endif /* CONFIG_OF_EARLY_FLATTREE */
+
+#ifdef CONFIG_ACPI_DBG2_TABLE
+
+int __init acpi_setup_earlycon(struct acpi_dbg2_device *device, void *d)
+{
+	int err;
+	struct uart_port *port = &early_console_dev.port;
+	int (*setup)(struct earlycon_device *, const char *) = d;
+	struct acpi_generic_address *reg;
+
+	if (!setup_dbg2_earlycon)
+		return -ENODEV;
+
+	if (device->register_count < 1)
+		return -ENODEV;
+
+	if (device->base_address_offset >= device->length)
+		return -EINVAL;
+
+	reg = (void *)device + device->base_address_offset;
+
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+	    reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO)
+		return -EINVAL;
+
+	spin_lock_init(&port->lock);
+	port->uartclk = BASE_BAUD * 16;
+
+	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		if (device->port_type == ACPI_DBG2_ARM_SBSA_32BIT)
+			port->iotype = UPIO_MEM32;
+		else
+			port->iotype = UPIO_MEM;
+		port->mapbase = reg->address;
+		port->membase = earlycon_map(reg->address, SZ_4K);
+	} else {
+		port->iotype = UPIO_PORT;
+		port->iobase = reg->address;
+	}
+
+	early_console_dev.con->data = &early_console_dev;
+	err = setup(&early_console_dev, NULL);
+	if (err < 0)
+		return err;
+	if (!early_console_dev.con->write)
+		return -ENODEV;
+
+	register_console(early_console_dev.con);
+	return 0;
+}
+
+#endif /* CONFIG_ACPI_DBG2_TABLE */

diff --git a/include/linux/acpi_dbg2.h b/include/linux/acpi_dbg2.h
index 125ae7e..b653752 100644
--- a/include/linux/acpi_dbg2.h
+++ b/include/linux/acpi_dbg2.h

@@ -37,12 +37,32 @@ int acpi_dbg2_setup(struct acpi_table_header *header, const void *data);
 	ACPI_DECLARE_PROBE_ENTRY(dbg2, name, ACPI_SIG_DBG2,		\
 				 acpi_dbg2_setup, &__acpi_dbg2_data_##name)
 
+int acpi_setup_earlycon(struct acpi_dbg2_device *device, void *d);
+
+/**
+ * ACPI_DBG2_EARLYCON_DECLARE() - Define handler for ACPI GDB2 serial port
+ * @name:		Identifier to compose name of table data
+ * @subtype:		Subtype of the port
+ * @console_setup:	Function to be called to setup the port
+ *
+ * Type of the console_setup() callback is
+ * int (*setup)(struct earlycon_device *, const char *)
+ * It's the type of callback of of_setup_earlycon().
+ */
+#define ACPI_DBG2_EARLYCON_DECLARE(name, subtype, console_setup)	\
+	ACPI_DBG2_DECLARE(name, ACPI_DBG2_SERIAL_PORT, subtype,		\
+			  acpi_setup_earlycon, console_setup)
+
 #else
 
 #define ACPI_DBG2_DECLARE(name, type, subtype, setup_fn, data_ptr)	\
 	static const void *__acpi_dbg_data_##name[]			\
 		__used __initdata = { (void *)setup_fn,	(void *)data_ptr }
 
+#define ACPI_DBG2_EARLYCON_DECLARE(name, subtype, console_setup)	\
+	static const void *__acpi_dbg_data_serial_##name[]		\
+		__used __initdata = { (void *)console_setup }

console_setup is a terrible macro argument name; console_setup() is an
actual kernel function (although file-scope).
Please change it to something short and generic.

Is 'setup_fn' ok?

Honestly, I'd just prefer you skip all this apparatus that makes
ACPI earlycon appear to be like OF earlycon code-wise, but without any of
the real underpinning or flexibility.

Actually it was Mark Salter who asked to introduce such macros.

https://lkml.kernel.org/g/1441730339.5459.8.camel at redhat.com

I think reusing the OF functions is a good decision.

Your "but without any of the real underpinning or flexibility" is unfounded.

This would be trivial to parse the ACPI table and invoke
setup_earlycon() with a string specifier instead.

For example,

int __init acpi_earlycon_setup(struct acpi_dbg2_device *dbg2)
{
        char opts[64];
        struct acpi_generic_addr *addr = (void*)dbg2 + dbg2->base_address_offset;
	int mmio = addr->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY;

        if (dbg2->port_type != ACPI_DBG2_SERIAL_PORT)
                return 0;

        switch (dbg2->port_subtype) {
        case ACPI_DBG2_ARM_PL011:
	case ACPI_DBG2_ARM_SBSA_GENERIC:
	case ACPI_DBG2_BCM2835:
                sprintf(opts, "pl011,%s,0x%llx", mmio, addr->address);
                break;
        case ACPI_DBG2_ARM_SBSA_32BIT:
                sprintf(opts, "pl011,mmio32,0x%llx", addr->address);
                break;
        case ACPI_DBG2_16550_COMPATIBLE:
        case ACPI_DBG2_16550_SUBSET:
                sprintf(opts, "uart,%s,0x%llx", mmio, addr->address);
                break;
        default:
                return 0;
        }

	return setup_earlycon(opts);
}

- Note that this decision forces setting earlycon on GDB2 debug port.
  DBG2 does not specify that it should be exactly earlycon.

- You missed ACPI_DBG2_ARM_DCC.  And actually I think the list of 
  debug ports is open. You will have to make up names like "uart" "pl011"
  each time a new port is introduced into the specs.

- Most important thing, this way you disclose the internals of serial ports
  to the generic earlycon.c  Such info as access mode should stay 
  in the respective drivers.

- I would not like printing address and then parsing it back.

This supports every earlycon ACPI DBG2 declares, not just the ARM_PL011
subtype of your series.

To support earlycon on other types of debug port just add literally one
string of code (as in pl011).

quoted

quoted

quoted

+
 #endif
 
 #endif