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 <[email protected]>
> ---
> 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.
> +
> 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 }
> +
> #endif
>
> #endif
>
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 <[email protected]>
>> ---
>> 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.
>> +
>> 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 }
>> +
>> #endif
>>
>> #endif
>>
>
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 <[email protected]>
>>> ---
>>> 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.
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.
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);
}
This supports every earlycon ACPI DBG2 declares, not just the ARM_PL011
subtype of your series.
>>> +
>>> #endif
>>>
>>> #endif
>>>
>>
On 03/03/2016 09:48 AM, 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 <[email protected]>
>>>> ---
>>>> 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.
>
> 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.
>
> 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;
This should be
char *mmio = (addr->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) ? "mmio" : "io";
>
> 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);
> }
>
> This supports every earlycon ACPI DBG2 declares, not just the ARM_PL011
> subtype of your series.
>
>
>
>>>> +
>>>> #endif
>>>>
>>>> #endif
>>>>
>>>
>
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 <[email protected]>
>>>> ---
>>>> 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/[email protected]
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
>>>>
>>>
>
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 <[email protected]>
>>>>> ---
>>>>> 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/[email protected]
>
> 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
>>>>>
>>>>
>>
On 03/04/2016 05:03 AM, Aleksey Makarov wrote:
> Actually it was Mark Salter who asked to introduce such macros.
>
> https://lkml.kernel.org/g/[email protected]
I wasn't copied on that series, sorry.
> I think reusing the OF functions is a good decision.
But you're not reusing the OF functions; you're duplicating them.