On 03/04/2016 05:03 AM, Aleksey Makarov wrote:

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

quoted

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?

quoted

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.

1. Lack of real underpinning.

Can't start an earlycon at early_param time to debug arch issues.
As a result, everyone will continue using command line for earlycon which
makes this series useless.


2. Lack of flexibility.

OF earlycon supports any new hardware simply by string matching w/o
requiring any approvals. I can add earlycon support for anything in
10 minutes.

ACPI earlycon for any new hardware requires approvals and spec changes.
2-3 months.



Founded.

quoted

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.

Obviously my example was simplified to point out how easy it is
to start an earlycon with a string specifier.

I assumed you would understand that

        if (!setup_dbg2_earlycon)
                return 0;

was omitted for clarity (or handled at a higher level).

- You missed ACPI_DBG2_ARM_DCC.

What is this?

Your series _only_ adds ACPI_DBG2_ARM_PL011 and none of the others.
If you add ACPI_DBG2_ARM_DCC support to your series, I'll add it
to my example.

 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.

5 new ports have been added in 1 decade. I think we can keep up with
that rate of change.

And you keep going on about "make up names". _For the last time_,
these "make up names" are the documented and defined console names for
earlycons. They will *never* change, because these same string
specifiers are used on the command line.

- 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.

??

My example function above doesn't go in "generic earlycon.c"
You leave it in ACPI where it belongs. setup_earlycon() is already global
scope, because like I've said repeatedly, it's already used by firmware to
start earlycons.

And I don't know what you mean by "access mode".

If you're referring to ["io","mmio","mmio32"], this is how earlycon is
specified and has been since the first patch. Besides your own patch decodes
and sets the iotype (UPIO_IO, UPIO_MEM, UPIO_MEM32) so I don't get what
you're objecting to here.

And if anything, the DBG2 table is under-specified.
Such things as endianness, register stride and i/o width are missing.

Not to mention line settings like baud rate, parity, stop bits, and flow
control.

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

The "parsing it back" is already implemented: that's how command line
earlycon works. That will never change.

And this method is already used by firmware other than OF.

quoted

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).

And as I've already shown, so does my way.
In 1/2 as much code, without macros or all the ACPI linker table changes.

quoted

quoted

quoted

quoted

+
 #endif
 
 #endif