| Message ID | 1456749726-30261-6-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linaro.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. > + > 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 <aleksey.makarov@linaro.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. >> + >> 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 <aleksey.makarov@linaro.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. 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 <aleksey.makarov@linaro.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. > > 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 <aleksey.makarov@linaro.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. > 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 <aleksey.makarov@linaro.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 >>>>> >>>> >>
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/1441730339.5459.8.camel@redhat.com 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.
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; + } + 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
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@linaro.org> --- Documentation/kernel-parameters.txt | 3 ++ drivers/tty/serial/earlycon.c | 60 +++++++++++++++++++++++++++++++++++++ include/linux/acpi_dbg2.h | 20 +++++++++++++ 3 files changed, 83 insertions(+)