Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1316536 > unrolled thread

[PATCH 2/3] ACPI: parse SPCR and enable matching console

Started byAleksey Makarov <aleksey.makarov@linaro.org>
First post2016-01-25 12:50 +0100
Last post2016-01-27 15:10 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-25 12:50 +0100
    Re: [PATCH 2/3] ACPI: parse SPCR and enable matching console Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-25 15:20 +0100
      Re: [PATCH 2/3] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-25 16:10 +0100
    Re: [PATCH 2/3] ACPI: parse SPCR and enable matching console Peter Hurley <peter@hurleysoftware.com> - 2016-01-25 17:40 +0100
      Re: [PATCH 2/3] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-27 15:10 +0100

#1316536 — [PATCH 2/3] ACPI: parse SPCR and enable matching console

FromAleksey Makarov <aleksey.makarov@linaro.org>
Date2016-01-25 12:50 +0100
Subject[PATCH 2/3] ACPI: parse SPCR and enable matching console
Message-ID<qUNOG-3ga-15@gated-at.bofh.it>
'ARM Server Base Boot Requiremets' [1] mention SPCR
(Serial Port Console Redirection Table) [2] as a mandatory ACPI table
that specifies the configuration of serial console.

Parse this table and check if any registered console match
the description.  If it does, enable that console.

To implement that, introduce a new member
int (*acpi_match)(struct console *, struct acpi_table_spcr *)
of struct console.  It allows drivers to check if they provide
a matching console device.

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
[2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 arch/arm64/Kconfig      |  1 +
 drivers/acpi/Kconfig    |  3 ++
 drivers/acpi/Makefile   |  1 +
 drivers/acpi/spcr.c     | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/console.h | 12 +++++++
 kernel/printk/printk.c  | 82 +++++++++++++++++++++++++++++++++++++----------
 6 files changed, 167 insertions(+), 17 deletions(-)
 create mode 100644 drivers/acpi/spcr.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 573bebc..bf31e3c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -4,6 +4,7 @@ config ARM64
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_PCI_HOST_GENERIC if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
+	select ACPI_SPCR_TABLE if ACPI
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index e315061..142a338 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -60,6 +60,9 @@ config ACPI_CCA_REQUIRED
 config IORT_TABLE
 	bool
 
+config ACPI_SPCR_TABLE
+	bool
+
 config ACPI_DEBUGGER
 	bool "AML debugger interface (EXPERIMENTAL)"
 	select ACPI_DEBUG
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 265eb90..8316859 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
 obj-$(CONFIG_IORT_TABLE) 	+= iort.o
+obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o
diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
new file mode 100644
index 0000000..ccb19a0
--- /dev/null
+++ b/drivers/acpi/spcr.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2012, Intel Corporation
+ * Copyright (c) 2015, Red Hat, Inc.
+ * Copyright (c) 2015, 2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#define pr_fmt(fmt) "ACPI: SPCR: " fmt
+
+#include <linux/acpi.h>
+#include <linux/console.h>
+#include <linux/kernel.h>
+
+static struct acpi_table_spcr *spcr_table;
+
+int console_acpi_match(struct console *c, char **options)
+{
+	int err;
+
+	if (!c->acpi_match)
+		return -ENODEV;
+
+	if (!spcr_table)
+		return -EAGAIN;
+
+	err = c->acpi_match(c, spcr_table);
+	if (err < 0)
+		return err;
+
+	if (options) {
+		switch (spcr_table->baud_rate) {
+		case 3:
+			*options = "9600";
+			break;
+		case 4:
+			*options = "19200";
+			break;
+		case 6:
+			*options = "57600";
+			break;
+		case 7:
+			*options = "115200";
+			break;
+		default:
+			*options = "";
+			break;
+		}
+	}
+
+	return err;
+}
+
+static int __init spcr_table_detect(void)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		const char *msg = acpi_format_exception(status);
+
+		pr_err("Failed to get table, %s\n", msg);
+		return -EINVAL;
+	}
+
+	if (table->revision < 2)
+		return -EOPNOTSUPP;
+
+	spcr_table = (struct acpi_table_spcr *)table;
+
+	pr_info("Console at 0x%016llx\n", spcr_table->serial_port.address);
+
+	acpi_register_consoles_try_again();
+
+	return 0;
+}
+
+arch_initcall(spcr_table_detect);
diff --git a/include/linux/console.h b/include/linux/console.h
index bd19434..94d0bd8 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -117,6 +117,7 @@ static inline int con_debug_leave(void)
 #define CON_BRL		(32) /* Used for a braille device */
 #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
 
+struct acpi_table_spcr;
 struct console {
 	char	name[16];
 	void	(*write)(struct console *, const char *, unsigned);
@@ -125,6 +126,7 @@ struct console {
 	void	(*unblank)(void);
 	int	(*setup)(struct console *, char *);
 	int	(*match)(struct console *, char *name, int idx, char *options);
+	int	(*acpi_match)(struct console *, struct acpi_table_spcr *);
 	short	flags;
 	short	index;
 	int	cflag;
@@ -132,6 +134,16 @@ struct console {
 	struct	 console *next;
 };
 
+#ifdef CONFIG_ACPI
+int console_acpi_match(struct console *c, char **options);
+#else
+static inline int console_acpi_match(struct console *c, char **options)
+{
+	return -ENODEV;
+}
+#endif
+void acpi_register_consoles_try_again(void);
+
 /*
  * for_each_console() allows you to iterate on each console
  */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 37e531f..3cf8cba 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2430,6 +2430,25 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static DEFINE_MUTEX(acpi_consoles_delayed_mutex);
+static struct console *acpi_consoles_delayed;
+
+void acpi_register_consoles_try_again(void)
+{
+	mutex_lock(&acpi_consoles_delayed_mutex);
+	while (acpi_consoles_delayed) {
+
+		struct console *c = acpi_consoles_delayed;
+
+		acpi_consoles_delayed = acpi_consoles_delayed->next;
+
+		mutex_unlock(&acpi_consoles_delayed_mutex);
+		register_console(c);
+		mutex_lock(&acpi_consoles_delayed_mutex);
+	}
+	mutex_unlock(&acpi_consoles_delayed_mutex);
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2538,8 +2557,30 @@ void register_console(struct console *newcon)
 		break;
 	}
 
-	if (!(newcon->flags & CON_ENABLED))
-		return;
+	if (!(newcon->flags & CON_ENABLED)) {
+		char *opts;
+		int err;
+
+		if (newcon->index < 0)
+			newcon->index = 0;
+
+		err = console_acpi_match(newcon, &opts);
+
+		if (err == -EAGAIN) {
+			mutex_lock(&acpi_consoles_delayed_mutex);
+			newcon->next = acpi_consoles_delayed;
+			acpi_consoles_delayed = newcon;
+			mutex_unlock(&acpi_consoles_delayed_mutex);
+			return;
+		} else if (err < 0) {
+			return;
+		} else {
+			if (newcon->setup && newcon->setup(newcon, opts) != 0)
+				return;
+			newcon->flags |= CON_ENABLED | CON_CONSDEV;
+			preferred_console = true;
+		}
+	}
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
@@ -2612,34 +2653,41 @@ void register_console(struct console *newcon)
 }
 EXPORT_SYMBOL(register_console);
 
+static int delete_from_console_list(struct console **list, struct console *c)
+{
+	while (*list) {
+		struct console *cur = *list;
+
+		if (cur == c) {
+			*list = cur->next;
+			return 0;
+		}
+		list = &cur->next;
+	}
+	return 1;
+}
+
 int unregister_console(struct console *console)
 {
-        struct console *a, *b;
 	int res;
 
 	pr_info("%sconsole [%s%d] disabled\n",
 		(console->flags & CON_BOOT) ? "boot" : "" ,
 		console->name, console->index);
 
+	mutex_lock(&acpi_consoles_delayed_mutex);
+	res = delete_from_console_list(&acpi_consoles_delayed, console);
+	mutex_unlock(&acpi_consoles_delayed_mutex);
+	if (res == 0)
+		return res;
+
 	res = _braille_unregister_console(console);
 	if (res)
 		return res;
 
-	res = 1;
 	console_lock();
-	if (console_drivers == console) {
-		console_drivers=console->next;
-		res = 0;
-	} else if (console_drivers) {
-		for (a=console_drivers->next, b=console_drivers ;
-		     a; b=a, a=b->next) {
-			if (a == console) {
-				b->next = a->next;
-				res = 0;
-				break;
-			}
-		}
-	}
+
+	res = delete_from_console_list(&console_drivers, console);
 
 	if (!res && (console->flags & CON_EXTENDED))
 		nr_ext_console_drivers--;
-- 
2.7.0

[toc] | [next] | [standalone]

#1316742

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-25 15:20 +0100
Message-ID<qUQ9P-5cg-11@gated-at.bofh.it>
In reply to#1316536
On Mon, Jan 25, 2016 at 1:45 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> 'ARM Server Base Boot Requiremets' [1] mention SPCR
> (Serial Port Console Redirection Table) [2] as a mandatory ACPI table
> that specifies the configuration of serial console.
>
> Parse this table and check if any registered console match
> the description.  If it does, enable that console.
>
> To implement that, introduce a new member
> int (*acpi_match)(struct console *, struct acpi_table_spcr *)
> of struct console.  It allows drivers to check if they provide
> a matching console device.
>
> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx

Can you split this to several patches?
I see preparatory patch in console code, i.e.
delete_from_console_list(), adding SPCR support to ACPI, enabling it.

>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  arch/arm64/Kconfig      |  1 +
>  drivers/acpi/Kconfig    |  3 ++
>  drivers/acpi/Makefile   |  1 +
>  drivers/acpi/spcr.c     | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/console.h | 12 +++++++
>  kernel/printk/printk.c  | 82 +++++++++++++++++++++++++++++++++++++----------
>  6 files changed, 167 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/acpi/spcr.c
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 573bebc..bf31e3c 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -4,6 +4,7 @@ config ARM64
>         select ACPI_GENERIC_GSI if ACPI
>         select ACPI_PCI_HOST_GENERIC if ACPI
>         select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> +       select ACPI_SPCR_TABLE if ACPI
>         select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>         select ARCH_HAS_ELF_RANDOMIZE
>         select ARCH_HAS_GCOV_PROFILE_ALL
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index e315061..142a338 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -60,6 +60,9 @@ config ACPI_CCA_REQUIRED
>  config IORT_TABLE
>         bool
>
> +config ACPI_SPCR_TABLE
> +       bool
> +
>  config ACPI_DEBUGGER
>         bool "AML debugger interface (EXPERIMENTAL)"
>         select ACPI_DEBUG
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 265eb90..8316859 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  obj-$(CONFIG_ACPI_BGRT)                += bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)    += cppc_acpi.o
>  obj-$(CONFIG_IORT_TABLE)       += iort.o
> +obj-$(CONFIG_ACPI_SPCR_TABLE)  += spcr.o
>
>  # processor has its own "processor." module_param namespace
>  processor-y                    := processor_driver.o
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> new file mode 100644
> index 0000000..ccb19a0
> --- /dev/null
> +++ b/drivers/acpi/spcr.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (c) 2012, Intel Corporation
> + * Copyright (c) 2015, Red Hat, Inc.
> + * Copyright (c) 2015, 2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#define pr_fmt(fmt) "ACPI: SPCR: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +
> +static struct acpi_table_spcr *spcr_table;
> +
> +int console_acpi_match(struct console *c, char **options)
> +{
> +       int err;
> +
> +       if (!c->acpi_match)
> +               return -ENODEV;
> +
> +       if (!spcr_table)
> +               return -EAGAIN;
> +
> +       err = c->acpi_match(c, spcr_table);
> +       if (err < 0)
> +               return err;
> +
> +       if (options) {
> +               switch (spcr_table->baud_rate) {
> +               case 3:
> +                       *options = "9600";
> +                       break;
> +               case 4:
> +                       *options = "19200";
> +                       break;
> +               case 6:
> +                       *options = "57600";
> +                       break;
> +               case 7:
> +                       *options = "115200";
> +                       break;
> +               default:
> +                       *options = "";
> +                       break;
> +               }
> +       }
> +
> +       return err;
> +}
> +
> +static int __init spcr_table_detect(void)
> +{
> +       struct acpi_table_header *table;
> +       acpi_status status;
> +
> +       if (acpi_disabled)
> +               return -ENODEV;
> +
> +       status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
> +       if (ACPI_FAILURE(status)) {
> +               const char *msg = acpi_format_exception(status);
> +
> +               pr_err("Failed to get table, %s\n", msg);
> +               return -EINVAL;
> +       }
> +
> +       if (table->revision < 2)
> +               return -EOPNOTSUPP;
> +
> +       spcr_table = (struct acpi_table_spcr *)table;
> +
> +       pr_info("Console at 0x%016llx\n", spcr_table->serial_port.address);
> +
> +       acpi_register_consoles_try_again();
> +
> +       return 0;
> +}
> +
> +arch_initcall(spcr_table_detect);
> diff --git a/include/linux/console.h b/include/linux/console.h
> index bd19434..94d0bd8 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -117,6 +117,7 @@ static inline int con_debug_leave(void)
>  #define CON_BRL                (32) /* Used for a braille device */
>  #define CON_EXTENDED   (64) /* Use the extended output format a la /dev/kmsg */
>
> +struct acpi_table_spcr;
>  struct console {
>         char    name[16];
>         void    (*write)(struct console *, const char *, unsigned);
> @@ -125,6 +126,7 @@ struct console {
>         void    (*unblank)(void);
>         int     (*setup)(struct console *, char *);
>         int     (*match)(struct console *, char *name, int idx, char *options);
> +       int     (*acpi_match)(struct console *, struct acpi_table_spcr *);
>         short   flags;
>         short   index;
>         int     cflag;
> @@ -132,6 +134,16 @@ struct console {
>         struct   console *next;
>  };
>
> +#ifdef CONFIG_ACPI
> +int console_acpi_match(struct console *c, char **options);
> +#else
> +static inline int console_acpi_match(struct console *c, char **options)
> +{
> +       return -ENODEV;
> +}
> +#endif
> +void acpi_register_consoles_try_again(void);
> +
>  /*
>   * for_each_console() allows you to iterate on each console
>   */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 37e531f..3cf8cba 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2430,6 +2430,25 @@ static int __init keep_bootcon_setup(char *str)
>
>  early_param("keep_bootcon", keep_bootcon_setup);
>
> +static DEFINE_MUTEX(acpi_consoles_delayed_mutex);
> +static struct console *acpi_consoles_delayed;
> +
> +void acpi_register_consoles_try_again(void)
> +{
> +       mutex_lock(&acpi_consoles_delayed_mutex);
> +       while (acpi_consoles_delayed) {
> +
> +               struct console *c = acpi_consoles_delayed;
> +
> +               acpi_consoles_delayed = acpi_consoles_delayed->next;
> +
> +               mutex_unlock(&acpi_consoles_delayed_mutex);
> +               register_console(c);
> +               mutex_lock(&acpi_consoles_delayed_mutex);
> +       }
> +       mutex_unlock(&acpi_consoles_delayed_mutex);
> +}
> +
>  /*
>   * The console driver calls this routine during kernel initialization
>   * to register the console printing procedure with printk() and to
> @@ -2538,8 +2557,30 @@ void register_console(struct console *newcon)
>                 break;
>         }
>
> -       if (!(newcon->flags & CON_ENABLED))
> -               return;
> +       if (!(newcon->flags & CON_ENABLED)) {
> +               char *opts;
> +               int err;
> +
> +               if (newcon->index < 0)
> +                       newcon->index = 0;
> +
> +               err = console_acpi_match(newcon, &opts);
> +
> +               if (err == -EAGAIN) {
> +                       mutex_lock(&acpi_consoles_delayed_mutex);
> +                       newcon->next = acpi_consoles_delayed;
> +                       acpi_consoles_delayed = newcon;
> +                       mutex_unlock(&acpi_consoles_delayed_mutex);
> +                       return;
> +               } else if (err < 0) {
> +                       return;
> +               } else {
> +                       if (newcon->setup && newcon->setup(newcon, opts) != 0)
> +                               return;
> +                       newcon->flags |= CON_ENABLED | CON_CONSDEV;
> +                       preferred_console = true;
> +               }
> +       }
>
>         /*
>          * If we have a bootconsole, and are switching to a real console,
> @@ -2612,34 +2653,41 @@ void register_console(struct console *newcon)
>  }
>  EXPORT_SYMBOL(register_console);
>
> +static int delete_from_console_list(struct console **list, struct console *c)
> +{
> +       while (*list) {
> +               struct console *cur = *list;
> +
> +               if (cur == c) {
> +                       *list = cur->next;
> +                       return 0;
> +               }
> +               list = &cur->next;
> +       }
> +       return 1;
> +}
> +
>  int unregister_console(struct console *console)
>  {
> -        struct console *a, *b;
>         int res;
>
>         pr_info("%sconsole [%s%d] disabled\n",
>                 (console->flags & CON_BOOT) ? "boot" : "" ,
>                 console->name, console->index);
>
> +       mutex_lock(&acpi_consoles_delayed_mutex);
> +       res = delete_from_console_list(&acpi_consoles_delayed, console);
> +       mutex_unlock(&acpi_consoles_delayed_mutex);
> +       if (res == 0)
> +               return res;
> +
>         res = _braille_unregister_console(console);
>         if (res)
>                 return res;
>
> -       res = 1;
>         console_lock();
> -       if (console_drivers == console) {
> -               console_drivers=console->next;
> -               res = 0;
> -       } else if (console_drivers) {
> -               for (a=console_drivers->next, b=console_drivers ;
> -                    a; b=a, a=b->next) {
> -                       if (a == console) {
> -                               b->next = a->next;
> -                               res = 0;
> -                               break;
> -                       }
> -               }
> -       }
> +
> +       res = delete_from_console_list(&console_drivers, console);
>
>         if (!res && (console->flags & CON_EXTENDED))
>                 nr_ext_console_drivers--;
> --
> 2.7.0
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1316790

FromAleksey Makarov <aleksey.makarov@linaro.org>
Date2016-01-25 16:10 +0100
Message-ID<qUQWd-5Ow-3@gated-at.bofh.it>
In reply to#1316742
On 25.01.2016 20:14, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 1:45 PM, Aleksey Makarov
> <aleksey.makarov@linaro.org> wrote:
>> 'ARM Server Base Boot Requiremets' [1] mention SPCR
>> (Serial Port Console Redirection Table) [2] as a mandatory ACPI table
>> that specifies the configuration of serial console.
>>
>> Parse this table and check if any registered console match
>> the description.  If it does, enable that console.
>>
>> To implement that, introduce a new member
>> int (*acpi_match)(struct console *, struct acpi_table_spcr *)
>> of struct console.  It allows drivers to check if they provide
>> a matching console device.
>>
>> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
>> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
>
> Can you split this to several patches?

I am not sure I should.

> I see preparatory patch in console code, i.e.
> delete_from_console_list(), adding SPCR support to ACPI, enabling it.

It would be difficult to justify delete_from_console_list() in a 
separate patches before the rest of the changes. And enabling SPCR in a 
separate patch also looks oddly for me.

It would be great to have other comments on this before I fix this in 
the next version.

Thank you

[toc] | [prev] | [next] | [standalone]

#1316969

FromPeter Hurley <peter@hurleysoftware.com>
Date2016-01-25 17:40 +0100
Message-ID<qUSlk-6Ia-7@gated-at.bofh.it>
In reply to#1316536
On 01/25/2016 03:45 AM, Aleksey Makarov wrote:
> 'ARM Server Base Boot Requiremets' [1] mention SPCR
> (Serial Port Console Redirection Table) [2] as a mandatory ACPI table
> that specifies the configuration of serial console.
> 
> Parse this table and check if any registered console match
> the description.  If it does, enable that console.
> 
> To implement that, introduce a new member
> int (*acpi_match)(struct console *, struct acpi_table_spcr *)
> of struct console.  It allows drivers to check if they provide
> a matching console device.

Many, many platform proms with all sorts of binary table layout are already
supported by the existing console infrastructure. Why is ACPI different, that
requires extensive (and messy) changes to console initialization?


How is this going to work with earlycon?


This commit log is missing the reasoning behind adding locks, refactoring
into delete_from_console_list(), and retry loops.


> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  arch/arm64/Kconfig      |  1 +
>  drivers/acpi/Kconfig    |  3 ++
>  drivers/acpi/Makefile   |  1 +
>  drivers/acpi/spcr.c     | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/console.h | 12 +++++++
>  kernel/printk/printk.c  | 82 +++++++++++++++++++++++++++++++++++++----------
>  6 files changed, 167 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/acpi/spcr.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 573bebc..bf31e3c 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -4,6 +4,7 @@ config ARM64
>  	select ACPI_GENERIC_GSI if ACPI
>  	select ACPI_PCI_HOST_GENERIC if ACPI
>  	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> +	select ACPI_SPCR_TABLE if ACPI
>  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>  	select ARCH_HAS_ELF_RANDOMIZE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index e315061..142a338 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -60,6 +60,9 @@ config ACPI_CCA_REQUIRED
>  config IORT_TABLE
>  	bool
>  
> +config ACPI_SPCR_TABLE
> +	bool
> +
>  config ACPI_DEBUGGER
>  	bool "AML debugger interface (EXPERIMENTAL)"
>  	select ACPI_DEBUG
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 265eb90..8316859 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
>  obj-$(CONFIG_IORT_TABLE) 	+= iort.o
> +obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> new file mode 100644
> index 0000000..ccb19a0
> --- /dev/null
> +++ b/drivers/acpi/spcr.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (c) 2012, Intel Corporation
> + * Copyright (c) 2015, Red Hat, Inc.
> + * Copyright (c) 2015, 2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#define pr_fmt(fmt) "ACPI: SPCR: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +
> +static struct acpi_table_spcr *spcr_table;
> +
> +int console_acpi_match(struct console *c, char **options)
> +{
> +	int err;
> +
> +	if (!c->acpi_match)
> +		return -ENODEV;
> +
> +	if (!spcr_table)
> +		return -EAGAIN;
> +
> +	err = c->acpi_match(c, spcr_table);
> +	if (err < 0)
> +		return err;
> +
> +	if (options) {
> +		switch (spcr_table->baud_rate) {
> +		case 3:
> +			*options = "9600";
> +			break;
> +		case 4:
> +			*options = "19200";
> +			break;
> +		case 6:
> +			*options = "57600";
> +			break;
> +		case 7:
> +			*options = "115200";
> +			break;
> +		default:
> +			*options = "";
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static int __init spcr_table_detect(void)
> +{
> +	struct acpi_table_header *table;
> +	acpi_status status;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
> +	if (ACPI_FAILURE(status)) {
> +		const char *msg = acpi_format_exception(status);
> +
> +		pr_err("Failed to get table, %s\n", msg);
> +		return -EINVAL;
> +	}
> +
> +	if (table->revision < 2)
> +		return -EOPNOTSUPP;
> +
> +	spcr_table = (struct acpi_table_spcr *)table;
> +
> +	pr_info("Console at 0x%016llx\n", spcr_table->serial_port.address);
> +
> +	acpi_register_consoles_try_again();
> +
> +	return 0;
> +}
> +
> +arch_initcall(spcr_table_detect);
> diff --git a/include/linux/console.h b/include/linux/console.h
> index bd19434..94d0bd8 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -117,6 +117,7 @@ static inline int con_debug_leave(void)
>  #define CON_BRL		(32) /* Used for a braille device */
>  #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
>  
> +struct acpi_table_spcr;
>  struct console {
>  	char	name[16];
>  	void	(*write)(struct console *, const char *, unsigned);
> @@ -125,6 +126,7 @@ struct console {
>  	void	(*unblank)(void);
>  	int	(*setup)(struct console *, char *);
>  	int	(*match)(struct console *, char *name, int idx, char *options);
> +	int	(*acpi_match)(struct console *, struct acpi_table_spcr *);
>  	short	flags;
>  	short	index;
>  	int	cflag;
> @@ -132,6 +134,16 @@ struct console {
>  	struct	 console *next;
>  };
>  
> +#ifdef CONFIG_ACPI
> +int console_acpi_match(struct console *c, char **options);
> +#else
> +static inline int console_acpi_match(struct console *c, char **options)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +void acpi_register_consoles_try_again(void);
> +
>  /*
>   * for_each_console() allows you to iterate on each console
>   */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 37e531f..3cf8cba 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2430,6 +2430,25 @@ static int __init keep_bootcon_setup(char *str)
>  
>  early_param("keep_bootcon", keep_bootcon_setup);
>  
> +static DEFINE_MUTEX(acpi_consoles_delayed_mutex);
> +static struct console *acpi_consoles_delayed;
> +
> +void acpi_register_consoles_try_again(void)
> +{
> +	mutex_lock(&acpi_consoles_delayed_mutex);
> +	while (acpi_consoles_delayed) {
> +
> +		struct console *c = acpi_consoles_delayed;
> +
> +		acpi_consoles_delayed = acpi_consoles_delayed->next;
> +
> +		mutex_unlock(&acpi_consoles_delayed_mutex);
> +		register_console(c);
> +		mutex_lock(&acpi_consoles_delayed_mutex);
> +	}
> +	mutex_unlock(&acpi_consoles_delayed_mutex);
> +}

Why is this necessary? There is no mention of this hack in the
commit log.


> +
>  /*
>   * The console driver calls this routine during kernel initialization
>   * to register the console printing procedure with printk() and to
> @@ -2538,8 +2557,30 @@ void register_console(struct console *newcon)
>  		break;
>  	}
>  
> -	if (!(newcon->flags & CON_ENABLED))
> -		return;
> +	if (!(newcon->flags & CON_ENABLED)) {
> +		char *opts;
> +		int err;
> +
> +		if (newcon->index < 0)
> +			newcon->index = 0;
> +
> +		err = console_acpi_match(newcon, &opts);
> +
> +		if (err == -EAGAIN) {
> +			mutex_lock(&acpi_consoles_delayed_mutex);
> +			newcon->next = acpi_consoles_delayed;
> +			acpi_consoles_delayed = newcon;
> +			mutex_unlock(&acpi_consoles_delayed_mutex);
> +			return;
> +		} else if (err < 0) {
> +			return;
> +		} else {
> +			if (newcon->setup && newcon->setup(newcon, opts) != 0)
> +				return;
> +			newcon->flags |= CON_ENABLED | CON_CONSDEV;
> +			preferred_console = true;
> +		}
> +	}
>  
>  	/*
>  	 * If we have a bootconsole, and are switching to a real console,
> @@ -2612,34 +2653,41 @@ void register_console(struct console *newcon)
>  }
>  EXPORT_SYMBOL(register_console);
>  
> +static int delete_from_console_list(struct console **list, struct console *c)
> +{
> +	while (*list) {
> +		struct console *cur = *list;
> +
> +		if (cur == c) {
> +			*list = cur->next;
> +			return 0;
> +		}
> +		list = &cur->next;
> +	}
> +	return 1;
> +}
> +
>  int unregister_console(struct console *console)
>  {
> -        struct console *a, *b;
>  	int res;
>  
>  	pr_info("%sconsole [%s%d] disabled\n",
>  		(console->flags & CON_BOOT) ? "boot" : "" ,
>  		console->name, console->index);
>  
> +	mutex_lock(&acpi_consoles_delayed_mutex);
> +	res = delete_from_console_list(&acpi_consoles_delayed, console);
> +	mutex_unlock(&acpi_consoles_delayed_mutex);
> +	if (res == 0)
> +		return res;
> +
>  	res = _braille_unregister_console(console);
>  	if (res)
>  		return res;
>  
> -	res = 1;
>  	console_lock();
> -	if (console_drivers == console) {
> -		console_drivers=console->next;
> -		res = 0;
> -	} else if (console_drivers) {
> -		for (a=console_drivers->next, b=console_drivers ;
> -		     a; b=a, a=b->next) {
> -			if (a == console) {
> -				b->next = a->next;
> -				res = 0;
> -				break;
> -			}
> -		}
> -	}
> +
> +	res = delete_from_console_list(&console_drivers, console);
>  
>  	if (!res && (console->flags & CON_EXTENDED))
>  		nr_ext_console_drivers--;
>

[toc] | [prev] | [next] | [standalone]

#1318994

FromAleksey Makarov <aleksey.makarov@linaro.org>
Date2016-01-27 15:10 +0100
Message-ID<qVyXj-4EQ-43@gated-at.bofh.it>
In reply to#1316969

On 01/25/2016 07:32 PM, Peter Hurley wrote:
> On 01/25/2016 03:45 AM, Aleksey Makarov wrote:
>> 'ARM Server Base Boot Requiremets' [1] mention SPCR (Serial Port
>> Console Redirection Table) [2] as a mandatory ACPI table that
>> specifies the configuration of serial console.
>> 
>> Parse this table and check if any registered console match the
>> description.  If it does, enable that console.
>> 
>> To implement that, introduce a new member int (*acpi_match)(struct
>> console *, struct acpi_table_spcr *) of struct console.  It allows
>> drivers to check if they provide a matching console device.
> 
> Many, many platform proms with all sorts of binary table layout are
> already supported by the existing console infrastructure. Why is ACPI
> different, that requires extensive (and messy) changes to console
> initialization?

Without this patch, when linux calls register_console(), that function
checks if any console has been enabled so far. 1) If not, it enables the
console being registered. 2) If there exists any enabled console, it
looks at the console_cmdline array. That array holds a list of
consoles that user wishes to enable. There are two ways to append
an item to that list: first is to pass "console=..." option in command
line and second is to call add_preferred_console(char *name, int idx,
char *options). As it is clear from the signature, the function
requires the name of the driver (like "ttyS") and the line id. On the
other hand, the SPCR ACPI table describes console by specifying the
address of it's registers or PCI Device ID / PCI Vendor ID or PCI Bus
Number / PCI Device Number. So to use this function we would need to
have a method to translate this info to the name of terminal and line
index. I could not figure out any way to do that.

In the initial version of the patch after getting the reference to the
SPCR ACPI table the full tree of ACPI devices was searched to find any
device with the same address.  When uart_add_one_port() was called
to register a new serial port, the ACPI companion of this port was
compared to the found device. If it was the same device, the code
called add_preferred_console() (the terminal name and line index are
known in uart_add_one_port()).

This original approach had two problems: 

    1) It works with the SPCR tables that describe consoles only by
the address of the registers. I do not think that consoles that are
described by PCI info will appear in the near future, but decided to
implement this in a generic way. I would like to discuss if this
decision was good.

    2) Wrong order of initialization.  Many console drivers have already
been registered by the time uart_add_one_port() adds an item to the
console_cmdline array.  There is a similar problem with my
implementation,  but having a dedicated acpi_match() callback I
believe made it simpler to circumwent.

That's why I believe we need to add a new funcion pointer to struct
console. On the other hand, I do not understand which existing
structure you are referring.

> How is this going to work with earlycon?

If an earlycon that matches SPCR is being registered, the code will enable it.
While it is harmless. Even so I will check for earlycon in the next version
of the patch set, thank you.

> This commit log is missing the reasoning behind adding locks,
> refactoring into delete_from_console_list(), and retry loops.

I will add this to the next verion of the series.

Thank you
Aleksey Makarov


>> [1]
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
>>
>> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
>> 
>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

[ .. ]

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web