Message-ID: <56D9C866.1060007@hurleysoftware.com>
Date:	Fri, 4 Mar 2016 09:39:50 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	Aleksey Makarov <aleksey.makarov@...aro.org>,
	linux-acpi@...r.kernel.org
Cc:	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Russell King <linux@....linux.org.uk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rjw@...ysocki.net>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	Graeme Gregory <graeme.gregory@...aro.org>,
	Al Stone <ahs3@...hat.com>,
	Christopher Covington <cov@...eaurora.org>,
	Matthias Brugger <mbrugger@...e.com>,
	Len Brown <lenb@...nel.org>, Arnd Bergmann <arnd@...db.de>,
	linux-arch@...r.kernel.org
Subject: Re: [PATCH v3 4/7] ACPI: parse DBG2 table

On 03/04/2016 04:19 AM, Aleksey Makarov wrote:
> On 03/03/2016 07:40 PM, Peter Hurley wrote:
>> On 03/01/2016 10:19 AM, Aleksey Makarov wrote:
>>> On 03/01/2016 08:22 PM, Peter Hurley wrote:
>>>>> On 03/01/2016 05:49 PM, Peter Hurley wrote:
>>>>>> On 02/29/2016 04:42 AM, Aleksey Makarov wrote:
>>>>>>> 'ARM Server Base Boot Requirements' [1] mentions DBG2 (Microsoft Debug
>>>>>>> Port Table 2) [2] as a mandatory ACPI table that specifies debug ports.
>>>>>>>
>>>>>>> - Implement macros
>>>>>>>
>>>>>>> 	ACPI_DBG2_DECLARE(name, type, subtype, setup_fn, data_ptr)
>>>>>>>
>>>>>>>   that defines a handler for the port of given type and subtype.
>>>>>>>
>>>>>>> - For each declared port that is also described in the ACPI DBG2 table
>>>>>>>   call the provided callback.
>>>>>>
>>>>>> On 02/22/2016 06:43 AM, Aleksey Makarov wrote:
>>>>>>> On 02/19/2016 08:20 PM, Christopher Covington wrote:
>>>>>>>> Can the device specified in DBG2 be used for both earlycon and KGDB? If it can only be used for one, let's make sure the choice of earlycon vs KGDB is intentional rather than accidental.
>>>>>>>
>>>>>>> I just sent the DBG2 series.  It enables an earlycon on DBG2 port with
>>>>>>> an "earlycon=acpi_dbg2" option (we can discuss particular name).
>>>>>>> If you need KGDB on that port just support it for that port in the kernel
>>>>>>> (i. e. add a new instance of ACPI_DBG2_DECLARE() macros for that port, see the patches)
>>>>>>> and change the command line options.
>>>>>>> I hope that is OK.  We could continue this discussion in the DBG2 thread.
>>>>>>
>>>>>> This method will not work for kgdb, since kgdb doesn't actually
>>>>>> implement the i/o but rather runs on top of a console.
>>>>>
>>>>> I see. Thank you for pointing this out.
>>>>>
>>>>> I don't have requirements to implement running kgdb over the serial port
>>>>> specified with DBG2.  This feature should be supported separately.
>>>>
>>>> And this takes us back full-circle to my initial point regarding
>>>> supporting earlycon via ACPI: which is that my view is earlycon should
>>>> be opt-in for any ACPI-specified console, rather than console via
>>>> SPCR and earlycon via DBG2.
>>>
>>> This is the main point on which we do not agree: 
>>> should SPCR start a new earlycon or match existing full-featured console.
>>
>> My point of view is not that SPCR should *only* start an earlycon
>> but rather *optionally also* start an earlycon.
> 
> Again, we have DBG2 to specify earlycon.
> SPCR specifies full-featured console.  DBG2 specifies earlycon.
> 
>> This is the existing behavior of every other firmware-specified console.
> 
> It SPCR + DBG2 works exactly as every other firmware-specified console

No it doesn't.

For OF, /chosen/stdout-path specifies the "full-featured console".
Adding "earlycon" to the command line starts an earlycon on the same
console, without firmware changes.

For PCDP, the firmware-specified console starts an earlycon and
registers a full-featured console for the same device.

Same for MIPS Malta.

In fact, there's not a single in-tree example of firmware that
uses one table/setting for earlycon and a different table/setting for
full-featured console.


> (well, after I change "earlycon=acpi_dbg2" to just "earlycon" back)
> plus it allows to specify console and earlycon separately.
> 
>>> But I do not see how this kgdb/DBG2 feature makes your point of view
>>> more founded.  Can you please elaborate?
>>
>> Well, my main concern is that other configurations that you have not
>> provided for will not be supportable at all because of the design choices
>> you're making here.
>>
>> For example, your current design does not allow for earlycon+console
>> on the SPCR port and debugger on DBG2 port. I think that's a problem.
> 
> It can not run earlycon+console on the SPCR port because earlycon
> is specified separately by DBG2.  And that is not a problem, just specify it.
> 
> As for debugger, it's not the object of these patchets.
> But I am sure it will not be hard to run it on DBG2, it's just not
> what these patches do.

No offense, but 2 emails ago you were sure it was as trivial
as a macro invocation, until I pointed out that wouldn't work.


>> Another concern is that, since you haven't accounted for options which
>> we'll want to implement in the near future, that a rewrite will be
>> necessary to implement those.
> 
> Correct.  I can not forecast any future design decision.  Nobody can.

Actually Chris already pointed out kgdb needs to run from DBG2.
I understand you don't want to implement that. However, it is a design
decision that has already been forecasted.

So no one's asking you to either forecast or implement it, only to
account for it in the design.

I think console + optional earlycon over the SPCR-specified device
and kgdb over the DBG2-specified device makes more sense,
but I'm also willing to accept a more flexible configuration of
options.

For example,
1: SPCR: console
2: SPCR + earlycon command line: console + earlycon
3: SPCR: console, DBG2 + earlycon command line: earlycon
4: SPCR: console, DBG2 + kgdb command line: console + kgdboc
5: DBG2 + kgdb command line: console + kgdb
6: DBG2 + earlycon + kgdb command line: earlycon + console + kgdboc

etc..


>> This framework is fairly heavyweight to already not have properly
>> considered how to start kgdb via DBG2.
> 
> There is no framework.  It's just a fix for ACPI tables and one simple
> callback function.

Documentation/kernel-parameters.txt | 3 +
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/acpi.c | 2 +
drivers/acpi/Kconfig | 3 +
drivers/acpi/Makefile | 1 +
drivers/acpi/dbg2.c | 88 +++++++++++++++++++++++++++++
drivers/acpi/scan.c | 44 ++++++++++-----
drivers/clocksource/arm_arch_timer.c | 3 +-
drivers/irqchip/irq-gic.c | 4 +-
drivers/of/fdt.c | 11 +---
drivers/tty/serial/amba-pl011.c | 3 +
drivers/tty/serial/earlycon.c | 61 ++++++++++++++++++++
include/acpi/actbl2.h | 5 ++
include/asm-generic/vmlinux.lds.h | 1 +
include/linux/acpi.h | 104 ++++++++++++++++++++++++-----------
include/linux/acpi_dbg2.h | 68 +++++++++++++++++++++++
include/linux/clocksource.h | 2 +-
include/linux/irqchip.h | 5 +-
include/linux/of_fdt.h | 2 +
19 files changed, 348 insertions(+), 63 deletions(-)
create mode 100644 drivers/acpi/dbg2.c
create mode 100644 include/linux/acpi_dbg2.h


>  No design decisions were made to restrict kdbg via DBG2.

A plausible outline for what is required for implementation on top
of what you have submitted would be sufficient; an assertion that
"it will not be hard" is not.


>>> On the contrary, if SPCR console is earlycon, we will not be able to
>>> run kgdb on it.
>>
>> I'm not sure what you mean by "run kgdb on it". What is "it" and why
>> would that be a problem?
> 
> kgdb can not run over earlycon.

Right. I'm the one that told you that.


>  If SPCR runs earlycon we can not run kgdb over it.

??

Right now I can run an earlycon, a console and kgdb on that console
all on the same device with OF.

Why is this a problem for ACPI?

If SPCR defines the console to start and the user also opts for earlycon
on the same device, kgdb will still be able to later start on the console.

As I explained, when a real console starts, the earlycon is disabled.


>>>>> Thank you
>>>>> Aleksey Makarov
>>>>>
>>>>>>> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
>>>>>>> [2] http://go.microsoft.com/fwlink/p/?LinkId=234837
>>>>>>>
>>>>>>> Signed-off-by: Aleksey Makarov <aleksey.makarov@...aro.org>
>>>>>>> ---
>>>>>>>  drivers/acpi/Kconfig              |  3 ++
>>>>>>>  drivers/acpi/Makefile             |  1 +
>>>>>>>  drivers/acpi/dbg2.c               | 88 +++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/asm-generic/vmlinux.lds.h |  1 +
>>>>>>>  include/linux/acpi_dbg2.h         | 48 +++++++++++++++++++++
>>>>>>>  5 files changed, 141 insertions(+)
>>>>>>>  create mode 100644 drivers/acpi/dbg2.c
>>>>>>>  create mode 100644 include/linux/acpi_dbg2.h
>>>>>>>
>>>>>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>>>>>> index 65fb483..660666e 100644
>>>>>>> --- a/drivers/acpi/Kconfig
>>>>>>> +++ b/drivers/acpi/Kconfig
>>>>>>> @@ -57,6 +57,9 @@ config ACPI_SYSTEM_POWER_STATES_SUPPORT
>>>>>>>  config ACPI_CCA_REQUIRED
>>>>>>>  	bool
>>>>>>>  
>>>>>>> +config ACPI_DBG2_TABLE
>>>>>>> +	bool
>>>>>>> +
>>>>>>>  config ACPI_DEBUGGER
>>>>>>>  	bool "AML debugger interface"
>>>>>>>  	select ACPI_DEBUG
>>>>>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>>>>>>> index 7395928..3b5f1ea 100644
>>>>>>> --- a/drivers/acpi/Makefile
>>>>>>> +++ b/drivers/acpi/Makefile
>>>>>>> @@ -83,6 +83,7 @@ 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_DEBUGGER_USER) += acpi_dbg.o
>>>>>>> +obj-$(CONFIG_ACPI_DBG2_TABLE)	+= dbg2.o
>>>>>>>  
>>>>>>>  # processor has its own "processor." module_param namespace
>>>>>>>  processor-y			:= processor_driver.o
>>>>>>> diff --git a/drivers/acpi/dbg2.c b/drivers/acpi/dbg2.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..0f0f6ca
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/acpi/dbg2.c
>>>>>>> @@ -0,0 +1,88 @@
>>>>>>> +/*
>>>>>>> + * Copyright (c) 2012, Intel Corporation
>>>>>>> + * 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: DBG2: " fmt
>>>>>>> +
>>>>>>> +#include <linux/acpi_dbg2.h>
>>>>>>> +#include <linux/acpi.h>
>>>>>>> +#include <linux/kernel.h>
>>>>>>> +
>>>>>>> +static const char * __init type2string(u16 type)
>>>>>>> +{
>>>>>>> +	switch (type) {
>>>>>>> +	case ACPI_DBG2_SERIAL_PORT:
>>>>>>> +		return "SERIAL";
>>>>>>> +	case ACPI_DBG2_1394_PORT:
>>>>>>> +		return "1394";
>>>>>>> +	case ACPI_DBG2_USB_PORT:
>>>>>>> +		return "USB";
>>>>>>> +	case ACPI_DBG2_NET_PORT:
>>>>>>> +		return "NET";
>>>>>>> +	default:
>>>>>>> +		return "?";
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const char * __init subtype2string(u16 subtype)
>>>>>>> +{
>>>>>>> +	switch (subtype) {
>>>>>>> +	case ACPI_DBG2_16550_COMPATIBLE:
>>>>>>> +		return "16550_COMPATIBLE";
>>>>>>> +	case ACPI_DBG2_16550_SUBSET:
>>>>>>> +		return "16550_SUBSET";
>>>>>>> +	case ACPI_DBG2_ARM_PL011:
>>>>>>> +		return "ARM_PL011";
>>>>>>> +	case ACPI_DBG2_ARM_SBSA_32BIT:
>>>>>>> +		return "ARM_SBSA_32BIT";
>>>>>>> +	case ACPI_DBG2_ARM_SBSA_GENERIC:
>>>>>>> +		return "ARM_SBSA_GENERIC";
>>>>>>> +	case ACPI_DBG2_ARM_DCC:
>>>>>>> +		return "ARM_DCC";
>>>>>>> +	case ACPI_DBG2_BCM2835:
>>>>>>> +		return "BCM2835";
>>>>>>> +	default:
>>>>>>> +		return "?";
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +int __init acpi_dbg2_setup(struct acpi_table_header *table, const void *data)
>>>>>>> +{
>>>>>>> +	struct acpi_table_dbg2 *dbg2 = (struct acpi_table_dbg2 *)table;
>>>>>>> +	struct acpi_dbg2_data *dbg2_data = (struct acpi_dbg2_data *)data;
>>>>>>> +	struct acpi_dbg2_device *dbg2_device, *dbg2_end;
>>>>>>> +	int i;
>>>>>>> +
>>>>>>> +	dbg2_device = ACPI_ADD_PTR(struct acpi_dbg2_device, dbg2,
>>>>>>> +				   dbg2->info_offset);
>>>>>>> +	dbg2_end = ACPI_ADD_PTR(struct acpi_dbg2_device, dbg2, table->length);
>>>>>>> +
>>>>>>> +	for (i = 0; i < dbg2->info_count; i++) {
>>>>>>> +		if (dbg2_device + 1 > dbg2_end) {
>>>>>>> +			pr_err("device pointer overflows, bad table\n");
>>>>>>> +			return 0;
>>>>>>> +		}
>>>>>>> +
>>>>>>> +		if (dbg2_device->port_type == dbg2_data->port_type &&
>>>>>>> +		    dbg2_device->port_subtype == dbg2_data->port_subtype) {
>>>>>>> +			if (dbg2_device->port_type == ACPI_DBG2_SERIAL_PORT)
>>>>>>> +				pr_info("debug port: SERIAL; subtype: %s\n",
>>>>>>> +				     subtype2string(dbg2_device->port_subtype));
>>>>>>> +			else
>>>>>>> +				pr_info("debug port: %s\n",
>>>>>>> +					type2string(dbg2_device->port_type));
>>>>>>> +			dbg2_data->setup(dbg2_device, dbg2_data->data);
>>>>>>> +		}
>>>>>>> +
>>>>>>> +		dbg2_device = ACPI_ADD_PTR(struct acpi_dbg2_device, dbg2_device,
>>>>>>> +					   dbg2_device->length);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>>>>>>> index 8f5a12a..8cc49ba 100644
>>>>>>> --- a/include/asm-generic/vmlinux.lds.h
>>>>>>> +++ b/include/asm-generic/vmlinux.lds.h
>>>>>>> @@ -526,6 +526,7 @@
>>>>>>>  	IRQCHIP_OF_MATCH_TABLE()					\
>>>>>>>  	ACPI_PROBE_TABLE(irqchip)					\
>>>>>>>  	ACPI_PROBE_TABLE(clksrc)					\
>>>>>>> +	ACPI_PROBE_TABLE(dbg2)						\
>>>>>>>  	EARLYCON_TABLE()
>>>>>>>  
>>>>>>>  #define INIT_TEXT							\
>>>>>>> diff --git a/include/linux/acpi_dbg2.h b/include/linux/acpi_dbg2.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000..125ae7e
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/linux/acpi_dbg2.h
>>>>>>> @@ -0,0 +1,48 @@
>>>>>>> +#ifndef _ACPI_DBG2_H_
>>>>>>> +#define _ACPI_DBG2_H_
>>>>>>> +
>>>>>>> +#ifdef CONFIG_ACPI_DBG2_TABLE
>>>>>>> +
>>>>>>> +#include <linux/kernel.h>
>>>>>>> +
>>>>>>> +struct acpi_dbg2_device;
>>>>>>> +struct acpi_table_header;
>>>>>>> +
>>>>>>> +struct acpi_dbg2_data {
>>>>>>> +	u16 port_type;
>>>>>>> +	u16 port_subtype;
>>>>>>> +	int (*setup)(struct acpi_dbg2_device *, void *);
>>>>>>> +	void *data;
>>>>>>> +};
>>>>>>> +
>>>>>>> +int acpi_dbg2_setup(struct acpi_table_header *header, const void *data);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * ACPI_DBG2_DECLARE() - Define handler for ACPI DBG2 port
>>>>>>> + * @name:	Identifier to compose name of table data
>>>>>>> + * @type:	Type of the port
>>>>>>> + * @subtype:	Subtype of the port
>>>>>>> + * @setup_fn:	Function to be called to setup the port
>>>>>>> + *		(of type int (*)(struct acpi_dbg2_device *, void *);)
>>>>>>> + * @data_ptr:	Sideband data provided back to the driver
>>>>>>> + */
>>>>>>> +#define ACPI_DBG2_DECLARE(name, type, subtype, setup_fn, data_ptr)	\
>>>>>>> +	static const struct acpi_dbg2_data				\
>>>>>>> +		__acpi_dbg2_data_##name __used = {			\
>>>>>>> +			.port_type = type,				\
>>>>>>> +			.port_subtype = subtype,			\
>>>>>>> +			.setup = setup_fn,				\
>>>>>>> +			.data = data_ptr,				\
>>>>>>> +		   };							\
>>>>>>> +	ACPI_DECLARE_PROBE_ENTRY(dbg2, name, ACPI_SIG_DBG2,		\
>>>>>>> +				 acpi_dbg2_setup, &__acpi_dbg2_data_##name)
>>>>>>> +
>>>>>>> +#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 }
>>>>>>> +
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +#endif
>>>>>>>
>>>>>>
>>>>
>>

Powered by blists - more mailing lists