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

Groups > linux.kernel > #1334687 > unrolled thread

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

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 v3 2/5] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-02-15 19:20 +0100
    Re: [PATCH v3 2/5] ACPI: parse SPCR and enable matching console Peter Hurley <peter@hurleysoftware.com> - 2016-02-18 23:20 +0100
      Re: [PATCH v3 2/5] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-02-19 11:50 +0100
        Re: [PATCH v3 2/5] ACPI: parse SPCR and enable matching console Peter Hurley <peter@hurleysoftware.com> - 2016-02-19 17:20 +0100
    Re: [PATCH v3 2/5] ACPI: parse SPCR and enable matching console Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-02-22 16:10 +0100

#1334687 — [PATCH v3 2/5] ACPI: parse SPCR and enable matching console

'ARM Server Base Boot Requiremets' [1] mentions 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.

Introduce a new function acpi_console_check().  At the uart port
registration, this function checks if the ACPI SPCR table specifies
its argument of type struct uart_port to be a console
and if so calls add_preferred_console().

[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>
---
 drivers/acpi/Kconfig             |  3 ++
 drivers/acpi/Makefile            |  1 +
 drivers/acpi/spcr.c              | 97 ++++++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/serial_core.c | 14 +++++-
 include/linux/acpi.h             | 10 +++++
 5 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100644 drivers/acpi/spcr.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 65fb483..5611eb6 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER
 
 endif
 
+config ACPI_SPCR_TABLE
+	bool
+
 config ACPI_SLEEP
 	bool
 	depends on SUSPEND || HIBERNATION
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 346101c..708b143 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
+obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
 
 # processor has its own "processor." module_param namespace
diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
new file mode 100644
index 0000000..a1eca91
--- /dev/null
+++ b/drivers/acpi/spcr.c
@@ -0,0 +1,97 @@
+/*
+ * 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>
+#include <linux/serial_core.h>
+
+static int acpi_table_parse_spcr(int (*handler)(struct acpi_table_spcr *table,
+						void *data), void *data)
+{
+	struct acpi_table_spcr *table = NULL;
+	acpi_size table_size;
+	acpi_status status;
+	int err;
+
+	status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0,
+					  (struct acpi_table_header **)&table,
+					  &table_size);
+
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	err = handler(table, data);
+
+	early_acpi_os_unmap_memory(table, table_size);
+
+	return err;
+}
+
+static int spcr_table_handler_check(struct acpi_table_spcr *table, void *data)
+{
+	struct uart_port *uport = data;
+	char *options;
+
+	if (table->header.revision < 2)
+		return -EOPNOTSUPP;
+
+	switch (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;
+	}
+
+	if ((table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+	     table->serial_port.address == (u64)uport->mapbase) ||
+	    (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
+	     table->serial_port.address == (u64)uport->iobase)) {
+		pr_info("adding preferred console [%s%d]\n", uport->cons->name,
+			uport->line);
+		add_preferred_console(uport->cons->name, uport->line, options);
+		return 1;
+
+	}
+
+	return 0;
+}
+
+/**
+ * acpi_console_check - Check if uart matches the console specified by SPCR.
+ *
+ * @uport:	uart port to check
+ *
+ * This function checks if the ACPI SPCR table specifies @uport to be a console
+ * and if so calls add_preferred_console()
+ *
+ * Return: a non-error value if the console matches.
+ */
+bool acpi_console_check(struct uart_port *uport)
+{
+	if (acpi_disabled || console_set_on_cmdline)
+		return false;
+
+	return acpi_table_parse_spcr(spcr_table_handler_check, uport) > 0;
+}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a126a60..459ab54 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -34,6 +34,7 @@
 #include <linux/serial_core.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
+#include <linux/acpi.h>
 
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		spin_lock_init(&uport->lock);
 		lockdep_set_class(&uport->lock, &port_lock_key);
 	}
-	if (uport->cons && uport->dev)
-		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
+
+	/*
+	 * Support both open FW and ACPI access to console definitions.
+	 * Both of_console_check() and acpi_console_check() will call
+	 * add_preferred_console() if a console definition is found.
+	 */
+	if (uport->cons && uport->dev) {
+		if (!acpi_console_check(uport))
+			of_console_check(uport->dev->of_node, uport->cons->name,
+					 uport->line);
+	}
 
 	uart_configure_port(drv, state, uport);
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 06ed7e5..ea0c297 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 #define acpi_probe_device_table(t)	({ int __r = 0; __r;})
 #endif
 
+struct uart_port;
+#ifdef CONFIG_ACPI_SPCR_TABLE
+bool acpi_console_check(struct uart_port *uport);
+#else
+static inline bool acpi_console_check(struct uart_port *uport)
+{
+	return FALSE;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
-- 
2.7.1

[toc] | [next] | [standalone]

#1337755

On 02/15/2016 10:05 AM, Aleksey Makarov wrote:
> 'ARM Server Base Boot Requiremets' [1] mentions 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.
> 
> Introduce a new function acpi_console_check().  At the uart port
> registration, this function checks if the ACPI SPCR table specifies
> its argument of type struct uart_port to be a console
> and if so calls add_preferred_console().
> 
> [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>
> ---
>  drivers/acpi/Kconfig             |  3 ++
>  drivers/acpi/Makefile            |  1 +
>  drivers/acpi/spcr.c              | 97 ++++++++++++++++++++++++++++++++++++++++
>  drivers/tty/serial/serial_core.c | 14 +++++-
>  include/linux/acpi.h             | 10 +++++
>  5 files changed, 123 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/acpi/spcr.c
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 65fb483..5611eb6 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER
>  
>  endif
>  
> +config ACPI_SPCR_TABLE
> +	bool
> +
>  config ACPI_SLEEP
>  	bool
>  	depends on SUSPEND || HIBERNATION
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 346101c..708b143 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
>  obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
> +obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>  
>  # processor has its own "processor." module_param namespace
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> new file mode 100644
> index 0000000..a1eca91
> --- /dev/null
> +++ b/drivers/acpi/spcr.c
> @@ -0,0 +1,97 @@
> +/*
> + * 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>
> +#include <linux/serial_core.h>
> +
> +static int acpi_table_parse_spcr(int (*handler)(struct acpi_table_spcr *table,
> +						void *data), void *data)
> +{
> +	struct acpi_table_spcr *table = NULL;
> +	acpi_size table_size;
> +	acpi_status status;
> +	int err;
> +
> +	status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0,
> +					  (struct acpi_table_header **)&table,
> +					  &table_size);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	err = handler(table, data);
> +
> +	early_acpi_os_unmap_memory(table, table_size);
> +
> +	return err;
> +}
> +
> +static int spcr_table_handler_check(struct acpi_table_spcr *table, void *data)
> +{
> +	struct uart_port *uport = data;
> +	char *options;
> +
> +	if (table->header.revision < 2)
> +		return -EOPNOTSUPP;
> +
> +	switch (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;
> +	}
> +
> +	if ((table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +	     table->serial_port.address == (u64)uport->mapbase) ||
> +	    (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
> +	     table->serial_port.address == (u64)uport->iobase)) {
> +		pr_info("adding preferred console [%s%d]\n", uport->cons->name,
> +			uport->line);
> +		add_preferred_console(uport->cons->name, uport->line, options);
> +		return 1;
> +
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * acpi_console_check - Check if uart matches the console specified by SPCR.
> + *
> + * @uport:	uart port to check
> + *
> + * This function checks if the ACPI SPCR table specifies @uport to be a console
> + * and if so calls add_preferred_console()
> + *
> + * Return: a non-error value if the console matches.
> + */
> +bool acpi_console_check(struct uart_port *uport)
> +{
> +	if (acpi_disabled || console_set_on_cmdline)
> +		return false;
> +
> +	return acpi_table_parse_spcr(spcr_table_handler_check, uport) > 0;
> +}
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a126a60..459ab54 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -34,6 +34,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/delay.h>
>  #include <linux/mutex.h>
> +#include <linux/acpi.h>
>  
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
> @@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  		spin_lock_init(&uport->lock);
>  		lockdep_set_class(&uport->lock, &port_lock_key);
>  	}
> -	if (uport->cons && uport->dev)
> -		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
> +
> +	/*
> +	 * Support both open FW and ACPI access to console definitions.
> +	 * Both of_console_check() and acpi_console_check() will call
> +	 * add_preferred_console() if a console definition is found.
> +	 */
> +	if (uport->cons && uport->dev) {
> +		if (!acpi_console_check(uport))
> +			of_console_check(uport->dev->of_node, uport->cons->name,
> +					 uport->line);

of_console_check() is not a good model for enabling a console.
It's a hack with several shortcomings (and bugs) that is tolerated here
because of its broad applicability to a wide range of platforms and drivers.

I specifically added console matching to enable exactly this kind of
console-selection-via-firmware. Please use that facility.

Regards,
Peter Hurley


> +	}
>  
>  	uart_configure_port(drv, state, uport);
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 06ed7e5..ea0c297 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
>  #define acpi_probe_device_table(t)	({ int __r = 0; __r;})
>  #endif
>  
> +struct uart_port;
> +#ifdef CONFIG_ACPI_SPCR_TABLE
> +bool acpi_console_check(struct uart_port *uport);
> +#else
> +static inline bool acpi_console_check(struct uart_port *uport)
> +{
> +	return FALSE;
> +}
> +#endif
> +
>  #endif	/*_LINUX_ACPI_H*/
> 

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

#1338043

Hi Peter,

Thank you for review.

On 02/19/2016 01:19 AM, Peter Hurley wrote:
> On 02/15/2016 10:05 AM, Aleksey Makarov wrote:

[..]

>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index a126a60..459ab54 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/serial_core.h>
>>  #include <linux/delay.h>
>>  #include <linux/mutex.h>
>> +#include <linux/acpi.h>
>>  
>>  #include <asm/irq.h>
>>  #include <asm/uaccess.h>
>> @@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>>  		spin_lock_init(&uport->lock);
>>  		lockdep_set_class(&uport->lock, &port_lock_key);
>>  	}
>> -	if (uport->cons && uport->dev)
>> -		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
>> +
>> +	/*
>> +	 * Support both open FW and ACPI access to console definitions.
>> +	 * Both of_console_check() and acpi_console_check() will call
>> +	 * add_preferred_console() if a console definition is found.
>> +	 */
>> +	if (uport->cons && uport->dev) {
>> +		if (!acpi_console_check(uport))
>> +			of_console_check(uport->dev->of_node, uport->cons->name,
>> +					 uport->line);
> 
> of_console_check() is not a good model for enabling a console.
> It's a hack with several shortcomings (and bugs) that is tolerated here
> because of its broad applicability to a wide range of platforms and drivers.

It's definitely not the best code I saw but there is no bugs.

> I specifically added console matching to enable exactly this kind of
> console-selection-via-firmware. Please use that facility.

I described why this does not work (except you insist on having 
new made up names for all types of SPCR consoles) in the reply to another your message.

Thank you
Aleksey Makarov

> Regards,
> Peter Hurley
> 
> 
>> +	}
>>  
>>  	uart_configure_port(drv, state, uport);
>>  
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 06ed7e5..ea0c297 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
>>  #define acpi_probe_device_table(t)	({ int __r = 0; __r;})
>>  #endif
>>  
>> +struct uart_port;
>> +#ifdef CONFIG_ACPI_SPCR_TABLE
>> +bool acpi_console_check(struct uart_port *uport);
>> +#else
>> +static inline bool acpi_console_check(struct uart_port *uport)
>> +{
>> +	return FALSE;
>> +}
>> +#endif
>> +
>>  #endif	/*_LINUX_ACPI_H*/
>>
> 

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

#1338319

On 02/19/2016 02:47 AM, Aleksey Makarov wrote:
> Hi Peter,
> 
> Thank you for review.
> 
> On 02/19/2016 01:19 AM, Peter Hurley wrote:
>> On 02/15/2016 10:05 AM, Aleksey Makarov wrote:
> 
> [..]
> 
>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>> index a126a60..459ab54 100644
>>> --- a/drivers/tty/serial/serial_core.c
>>> +++ b/drivers/tty/serial/serial_core.c
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/serial_core.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/mutex.h>
>>> +#include <linux/acpi.h>
>>>  
>>>  #include <asm/irq.h>
>>>  #include <asm/uaccess.h>
>>> @@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>>>  		spin_lock_init(&uport->lock);
>>>  		lockdep_set_class(&uport->lock, &port_lock_key);
>>>  	}
>>> -	if (uport->cons && uport->dev)
>>> -		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
>>> +
>>> +	/*
>>> +	 * Support both open FW and ACPI access to console definitions.
>>> +	 * Both of_console_check() and acpi_console_check() will call
>>> +	 * add_preferred_console() if a console definition is found.
>>> +	 */
>>> +	if (uport->cons && uport->dev) {
>>> +		if (!acpi_console_check(uport))
>>> +			of_console_check(uport->dev->of_node, uport->cons->name,
>>> +					 uport->line);
>>
>> of_console_check() is not a good model for enabling a console.
>> It's a hack with several shortcomings (and bugs) that is tolerated here
>> because of its broad applicability to a wide range of platforms and drivers.
> 
> It's definitely not the best code I saw 

> but there is no bugs.

I pointed out just one of the problems with adding a preferred console
late in the boot cycle (eg, at uart driver load) in the other patch review.


>> I specifically added console matching to enable exactly this kind of
>> console-selection-via-firmware. Please use that facility.
> 
> I described why this does not work (except you insist on having 
> new made up names for all types of SPCR consoles) in the reply to another your message.

They're not "new made up names".

They're the documented names of earlycons, which you'd know if you
researched how you're going to start an earlycon with the DBG2 table
like I suggested in previous reviews.

And I explained in the other patch review exactly how it works.

Regards,
Peter Hurley


>>> +	}
>>>  
>>>  	uart_configure_port(drv, state, uport);
>>>  
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index 06ed7e5..ea0c297 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
>>>  #define acpi_probe_device_table(t)	({ int __r = 0; __r;})
>>>  #endif
>>>  
>>> +struct uart_port;
>>> +#ifdef CONFIG_ACPI_SPCR_TABLE
>>> +bool acpi_console_check(struct uart_port *uport);
>>> +#else
>>> +static inline bool acpi_console_check(struct uart_port *uport)
>>> +{
>>> +	return FALSE;
>>> +}
>>> +#endif
>>> +
>>>  #endif	/*_LINUX_ACPI_H*/
>>>
>>

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

#1339615

Hi Yury,

Thank you for review/suggestions.  I will address it all in the next version except for...

On 02/21/2016 12:42 PM, Yury Norov wrote:
> On Mon, Feb 15, 2016 at 09:05:26PM +0300, Aleksey Makarov wrote:
>> 'ARM Server Base Boot Requiremets' [1] mentions 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.
>>
>> Introduce a new function acpi_console_check().  At the uart port
>> registration, this function checks if the ACPI SPCR table specifies
>> its argument of type struct uart_port to be a console
>> and if so calls add_preferred_console().
>>
>> [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>
>> ---
>>  drivers/acpi/Kconfig             |  3 ++
>>  drivers/acpi/Makefile            |  1 +
>>  drivers/acpi/spcr.c              | 97 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/tty/serial/serial_core.c | 14 +++++-
>>  include/linux/acpi.h             | 10 +++++
>>  5 files changed, 123 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/acpi/spcr.c
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 65fb483..5611eb6 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER
>>  
>>  endif
>>  
>> +config ACPI_SPCR_TABLE
>> +	bool
>> +
>>  config ACPI_SLEEP
>>  	bool
>>  	depends on SUSPEND || HIBERNATION
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 346101c..708b143 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
>>  obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>>  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
>> +obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>>  
>>  # processor has its own "processor." module_param namespace
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>> new file mode 100644
>> index 0000000..a1eca91
>> --- /dev/null
>> +++ b/drivers/acpi/spcr.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * 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>
>> +#include <linux/serial_core.h>
>> +
>> +static int acpi_table_parse_spcr(int (*handler)(struct acpi_table_spcr *table,
>> +						void *data), void *data)
>> +{
>> +	struct acpi_table_spcr *table = NULL;
> Hi Alexey,
> 
> Few minor comments...
> 
> Are you sure you need thin initialization?
> 
>> +	acpi_size table_size;
>> +	acpi_status status;
>> +	int err;
>> +
>> +	status = acpi_get_table_with_size(ACPI_SIG_SPCR, 0,
>> +					  (struct acpi_table_header **)&table,
>> +					  &table_size);
>> +
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>> +
>> +	err = handler(table, data);
>> +
>> +	early_acpi_os_unmap_memory(table, table_size);
>> +
>> +	return err;
>> +}
>> +
>> +static int spcr_table_handler_check(struct acpi_table_spcr *table, void *data)
> 
> The name of function is not clear for me.
> You're not only checking here, but also
> adding console. 
> 
>> +{
>> +	struct uart_port *uport = data;
>> +	char *options;
>> +
>> +	if (table->header.revision < 2)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (table->baud_rate) {
> 
> You don't need 'options' if your big condition returns false,
> so you can evaluate it inside conditional block.
> 
>> +	case 3:
>> +		options = "9600";
>> +		break;
>> +	case 4:
>> +		options = "19200";
>> +		break;
>> +	case 6:
>> +		options = "57600";
>> +		break;
>> +	case 7:
>> +		options = "115200";
>> +		break;
>> +	default:
>> +		options = "";
>> +		break;
> 
> 'break' isnot needed here

... except for this.

No, it is not, but C language style guides tell us to have it here.
Also see the first example from Documentation/CodingStyle 
where they have it.

Thank you
Aleksey Makarov

> 
>> +	}
>> +
>> +	if ((table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> 
> Nitpick: just for better readability, I'd split it onto a set of
> separated checks:
> 
>         foo() {
>                 if (!cond1)
>                         return 0;
> 
>                 if (!cond2)
>                         return 0;
>                 ...
> 
>                 switch () {
>                 case 1:
>                         ...
>                 case 2:
>                         ...
>                 case 3:
>                         ...
>                 default:
>                         ...
>                 }
>                 
>                 return 1;
>         }
> 
>> +	     table->serial_port.address == (u64)uport->mapbase) ||
>> +	    (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
>> +	     table->serial_port.address == (u64)uport->iobase)) {
>> +		pr_info("adding preferred console [%s%d]\n", uport->cons->name,
>> +			uport->line);
>> +		add_preferred_console(uport->cons->name, uport->line, options);
>> +		return 1;
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * acpi_console_check - Check if uart matches the console specified by SPCR.
>> + *
>> + * @uport:	uart port to check
>> + *
>> + * This function checks if the ACPI SPCR table specifies @uport to be a console
>> + * and if so calls add_preferred_console()
>> + *
>> + * Return: a non-error value if the console matches.
>> + */
>> +bool acpi_console_check(struct uart_port *uport)
>> +{
>> +	if (acpi_disabled || console_set_on_cmdline)
>> +		return false;
>> +
>> +	return acpi_table_parse_spcr(spcr_table_handler_check, uport) > 0;
>> +}
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index a126a60..459ab54 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/serial_core.h>
>>  #include <linux/delay.h>
>>  #include <linux/mutex.h>
>> +#include <linux/acpi.h>
>>  
>>  #include <asm/irq.h>
>>  #include <asm/uaccess.h>
>> @@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>>  		spin_lock_init(&uport->lock);
>>  		lockdep_set_class(&uport->lock, &port_lock_key);
>>  	}
>> -	if (uport->cons && uport->dev)
>> -		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
>> +
>> +	/*
>> +	 * Support both open FW and ACPI access to console definitions.
>> +	 * Both of_console_check() and acpi_console_check() will call
>> +	 * add_preferred_console() if a console definition is found.
>> +	 */
>> +	if (uport->cons && uport->dev) {
>> +		if (!acpi_console_check(uport))
>> +			of_console_check(uport->dev->of_node, uport->cons->name,
>> +					 uport->line);
>> +	}
>>  
>>  	uart_configure_port(drv, state, uport);
>>  
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 06ed7e5..ea0c297 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1004,4 +1004,14 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
>>  #define acpi_probe_device_table(t)	({ int __r = 0; __r;})
>>  #endif
>>  
>> +struct uart_port;
>> +#ifdef CONFIG_ACPI_SPCR_TABLE
>> +bool acpi_console_check(struct uart_port *uport);
>> +#else
>> +static inline bool acpi_console_check(struct uart_port *uport)
>> +{
>> +	return FALSE;
>> +}
>> +#endif
>> +
>>  #endif	/*_LINUX_ACPI_H*/
>> -- 
>> 2.7.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web