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 08:24 AM, Aleksey Makarov 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 (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. > 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. > 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. No design decisions were made to restrict kdbg via DBG2. >> 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. If SPCR runs earlycon we can not run kgdb over it. >>>> 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.maka...@linaro.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 >>>>>> >>>>> >>> >
