Date:	Fri, 4 Mar 2016 07:40:25 -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>,
	Jonathan Corbet <corbet@....net>, Jiri Slaby <jslaby@...e.com>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 5/7] ACPI: serial: implement earlycon on ACPI DBG2 port

On 03/04/2016 05:03 AM, Aleksey Makarov wrote:
> 
> 
> 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 <aleksey.makarov@...aro.org>
>>>>> ---
>>>>>  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()
>>
>>
>>>>> +
>>>>>  	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.

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.



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


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

>>>>> +
>>>>>  #endif
>>>>>  
>>>>>  #endif
>>>>>
>>>>
>>

Powered by blists - more mailing lists