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@xxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxx
>
> 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
>>>>>
>>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
