Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752693AbcCDNE6 (ORCPT ); Fri, 4 Mar 2016 08:04:58 -0500 Received: from mail-lb0-f170.google.com ([209.85.217.170]:35492 "EHLO mail-lb0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751294AbcCDNEx (ORCPT ); Fri, 4 Mar 2016 08:04:53 -0500 Subject: Re: [PATCH v3 5/7] ACPI: serial: implement earlycon on ACPI DBG2 port To: Peter Hurley , linux-acpi@vger.kernel.org References: <1456749726-30261-1-git-send-email-aleksey.makarov@linaro.org> <1456749726-30261-6-git-send-email-aleksey.makarov@linaro.org> <56D5BAFF.6080008@hurleysoftware.com> <56D5C9F5.5060301@linaro.org> <56D878D4.4000902@hurleysoftware.com> Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Russell King , Greg Kroah-Hartman , "Rafael J . Wysocki" , Leif Lindholm , Graeme Gregory , Al Stone , Christopher Covington , Matthias Brugger , Jonathan Corbet , Jiri Slaby , linux-doc@vger.kernel.org From: Aleksey Makarov Message-ID: <56D987BC.7070209@linaro.org> Date: Fri, 4 Mar 2016 16:03:56 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <56D878D4.4000902@hurleysoftware.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8918 Lines: 262 On 03/03/2016 08:48 PM, Peter Hurley wrote: > On 03/01/2016 08:57 AM, Aleksey Makarov wrote: >> >> >> On 03/01/2016 06:53 PM, Peter Hurley wrote: >>> On 02/29/2016 04:42 AM, Aleksey Makarov wrote: >>>> 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 >>>> --- >>>> 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 >>>> #include >>>> #include >>>> +#include >>>> >>>> #ifdef CONFIG_FIX_EARLYCON_MEM >>>> #include >>>> @@ -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() > > >>>> + >>>> 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@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). > > > >>>> + >>>> #endif >>>> >>>> #endif >>>> >>> >