[PATCH v2 0/3] Add earlycon support for AMD Carrizo / Stoneyridge

[PATCH v2 0/3] Add earlycon support for AMD Carrizo / Stoneyridge

[Daniel Kurtz]

AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a 48 MHz
input clock.  Currently, there is no way to tell earlycon to use a specific
uart input clock when configuring a baud rate.

Patch 1 adds an earlycon ->setup() callback to set up this clock.  The setup is
triggered on the commandline by specifying "earlycon=amdcz,...".

Patch 2 adds an ACPI SPCR quirk to automatically trigger this hook when the
ACPI SPCR table OEMID field is "AMDCZ ".

Patch 3 adds an additional fix which allows botht the ACPI SPCR quirk and amdcz
->setup() to tell 8250_core to skip initialization of old serial ports that do
not exist on these SoCs.

Daniel Kurtz (3):
  serial: 8250_early: Add earlycon support for AMD Carrizo / Stoneyridge
  ACPI: SPCR: Add support for AMD CT/SZ
  serial: core: Allow skipping old serial port initialization

 drivers/acpi/spcr.c                  | 25 +++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_core.c  |  6 ++++++
 drivers/tty/serial/8250/8250_early.c | 16 ++++++++++++++++
 include/linux/serial_8250.h          |  6 ++++++
 4 files changed, 53 insertions(+)

-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 1/3] serial: 8250_early: Add earlycon support for AMD Carrizo / Stoneyridge

[Daniel Kurtz]

AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a 48 MHz
input clock.

Allow these platforms to set up this clock by specifying a kernel command
line like:
earlycon=amdcz,mmio32,0xfedc6000,115200

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/tty/serial/8250/8250_early.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index ae6a256524d8..c6bf971a6038 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -195,3 +195,18 @@ static int __init early_au_setup(struct earlycon_device *dev, const char *opt)
 OF_EARLYCON_DECLARE(palmchip, "ralink,rt2880-uart", early_au_setup);
 
 #endif
+
+#ifdef CONFIG_SERIAL_8250_DW
+static int __init early_amdcz_setup(struct earlycon_device *dev,
+				    const char *opt)
+{
+	struct uart_port *port = &dev->port;
+
+	port->uartclk = 48000000;
+
+	return early_serial8250_setup(dev, opt);
+}
+
+EARLYCON_DECLARE(amdcz, early_amdcz_setup);
+
+#endif
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 1/3] serial: 8250_early: Add earlycon support for AMD Carrizo / Stoneyridge

[Daniel Kurtz]

AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a 48 MHz
input clock.

Allow these platforms to set up this clock by specifying a kernel command
line like:
earlycon=amdcz,mmio32,0xfedc6000,115200

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/tty/serial/8250/8250_early.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index ae6a256524d8..c6bf971a6038 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -195,3 +195,18 @@ static int __init early_au_setup(struct earlycon_device *dev, const char *opt)
 OF_EARLYCON_DECLARE(palmchip, "ralink,rt2880-uart", early_au_setup);
 
 #endif
+
+#ifdef CONFIG_SERIAL_8250_DW
+static int __init early_amdcz_setup(struct earlycon_device *dev,
+				    const char *opt)
+{
+	struct uart_port *port = &dev->port;
+
+	port->uartclk = 48000000;
+
+	return early_serial8250_setup(dev, opt);
+}
+
+EARLYCON_DECLARE(amdcz, early_amdcz_setup);
+
+#endif
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 2/3] ACPI: SPCR: Add support for AMD CT/SZ

[Daniel Kurtz]

AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a special
earlycon setup handler to configure its input clock in order to compute
baud rate divisor registers.
Detect them by examining the OEMID field in the SPCR header, and pass
then pass uart type amdcz to earlycon.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/acpi/spcr.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 9d52743080a4..52d840d0e05b 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -73,6 +73,24 @@ static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
 	return xgene_8250;
 }
 
+/*
+ * AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a special
+ * earlycon setup handler to configure its input clock in order to compute
+ * baud rate divisor registers.
+ * Detect them by examining the OEM fields in the SPCR header.
+ */
+static bool amdcz_present(struct acpi_table_spcr *tb)
+{
+	if (memcmp(tb->header.oem_id, "AMDCZ ", ACPI_OEM_ID_SIZE))
+		return false;
+
+	if (memcmp(tb->header.oem_table_id, "AMDCZ   ",
+		    ACPI_OEM_TABLE_ID_SIZE))
+		return false;
+
+	return true;
+}
+
 /**
  * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -189,6 +207,11 @@ int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
 			uart = "qdf2400_e44";
 	}
 
+	if (amdcz_present(table)) {
+		if (enable_earlycon)
+			uart = "amdcz";
+	}
+
 	if (xgene_8250_erratum_present(table)) {
 		iotype = "mmio32";
 
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 2/3] ACPI: SPCR: Add support for AMD CT/SZ

[Daniel Kurtz]

AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a special
earlycon setup handler to configure its input clock in order to compute
baud rate divisor registers.
Detect them by examining the OEMID field in the SPCR header, and pass
then pass uart type amdcz to earlycon.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/acpi/spcr.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 9d52743080a4..52d840d0e05b 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -73,6 +73,24 @@ static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
 	return xgene_8250;
 }
 
+/*
+ * AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a special
+ * earlycon setup handler to configure its input clock in order to compute
+ * baud rate divisor registers.
+ * Detect them by examining the OEM fields in the SPCR header.
+ */
+static bool amdcz_present(struct acpi_table_spcr *tb)
+{
+	if (memcmp(tb->header.oem_id, "AMDCZ ", ACPI_OEM_ID_SIZE))
+		return false;
+
+	if (memcmp(tb->header.oem_table_id, "AMDCZ   ",
+		    ACPI_OEM_TABLE_ID_SIZE))
+		return false;
+
+	return true;
+}
+
 /**
  * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -189,6 +207,11 @@ int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
 			uart = "qdf2400_e44";
 	}
 
+	if (amdcz_present(table)) {
+		if (enable_earlycon)
+			uart = "amdcz";
+	}
+
 	if (xgene_8250_erratum_present(table)) {
 		iotype = "mmio32";
 
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Daniel Kurtz]

The old_serial_port global array in 8250_core is supposed to hold an entry
for each serial port on the system that cannot be discovered via a
standard enumeration mechanism (aka ACPI/PCI/DTS).  The array is populated
at compile-time from the value specified in the SERIAL_PORT_DFNS macro.
This macro is defined in arch/serial.h.

For x86, this macro is currently unconditionally initialized to supply
four ioport UARTs (0x3F8, 0x2F8, 0x3E8, 0x2E8).

However, not all x86 CPUs have these four ioport UARTs.  For example, the
UARTs on AMD Carrizo and later are separate memory mapped Designware IP
blocks.

Fairly early in boot the console_initcall univ8250_console_init iterates
over this array and installs these old UARTs into the global array
serial8250_ports.  Further, it attempts to register them for use as
the console.  In other words, if, for example, the kernel commandline has
console=ttyS0, the console will be switched over to one of these
non-existent UARTs.  Only later, when the real UART drivers are probed
and their devices are instantiated will the console switch back over to
the proper UART.

This is noticeable when using earlycon, since part of the serial console
log will appear to disappear (when the bogus old takes over) and then
re-appear (when the real UART finally gets registered for the console).

The problem is even more noticable when *not* using earlycon, since in
this case the entire console output is missing, having been incorrectly
played back to the non-existing serial port.

Create a global variable to allow skipping old serial port initialization
and wire it up to the AMDCZ ACPI SPCR quirk and the special amdcz earlycon
setup handler.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
Changes since v1:
 * Rename variable to serial8250_skip_old_ports
 * Also set variable in acpi/spcr. to support no-earlycon case.

 drivers/acpi/spcr.c                  | 2 ++
 drivers/tty/serial/8250/8250_core.c  | 6 ++++++
 drivers/tty/serial/8250/8250_early.c | 1 +
 include/linux/serial_8250.h          | 6 ++++++
 4 files changed, 15 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 52d840d0e05b..f59591283410 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/console.h>
 #include <linux/kernel.h>
+#include <linux/serial_8250.h>
 #include <linux/serial_core.h>
 
 /*
@@ -208,6 +209,7 @@ int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
 	}
 
 	if (amdcz_present(table)) {
+		serial8250_skip_old_ports = true;
 		if (enable_earlycon)
 			uart = "amdcz";
 	}
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..a04da392a251 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -66,6 +66,9 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
 #define SERIAL_PORT_DFNS
 #endif
 
+bool serial8250_skip_old_ports;
+EXPORT_SYMBOL(serial8250_skip_old_ports);
+
 static const struct old_serial_port old_serial_port[] = {
 	SERIAL_PORT_DFNS /* defined in asm/serial.h */
 };
@@ -537,6 +540,9 @@ static void __init serial8250_isa_init_ports(void)
 	if (share_irqs)
 		irqflag = IRQF_SHARED;
 
+	if (serial8250_skip_old_ports)
+		return;
+
 	for (i = 0, up = serial8250_ports;
 	     i < ARRAY_SIZE(old_serial_port) && i < nr_uarts;
 	     i++, up++) {
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index c6bf971a6038..288d2be82990 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -202,6 +202,7 @@ static int __init early_amdcz_setup(struct earlycon_device *dev,
 {
 	struct uart_port *port = &dev->port;
 
+	serial8250_skip_old_ports = true;
 	port->uartclk = 48000000;
 
 	return early_serial8250_setup(dev, opt);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a27ef5f56431..02570edaddd8 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -136,6 +136,12 @@ struct uart_8250_port {
 	struct uart_8250_em485 *em485;
 };
 
+#ifdef CONFIG_SERIAL_8250
+extern bool serial8250_skip_old_ports;
+#else
+static const bool serial8250_skip_old_ports;
+#endif
+
 static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
 {
 	return container_of(up, struct uart_8250_port, port);
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Daniel Kurtz]

The old_serial_port global array in 8250_core is supposed to hold an entry
for each serial port on the system that cannot be discovered via a
standard enumeration mechanism (aka ACPI/PCI/DTS).  The array is populated
at compile-time from the value specified in the SERIAL_PORT_DFNS macro.
This macro is defined in arch/serial.h.

For x86, this macro is currently unconditionally initialized to supply
four ioport UARTs (0x3F8, 0x2F8, 0x3E8, 0x2E8).

However, not all x86 CPUs have these four ioport UARTs.  For example, the
UARTs on AMD Carrizo and later are separate memory mapped Designware IP
blocks.

Fairly early in boot the console_initcall univ8250_console_init iterates
over this array and installs these old UARTs into the global array
serial8250_ports.  Further, it attempts to register them for use as
the console.  In other words, if, for example, the kernel commandline has
console=ttyS0, the console will be switched over to one of these
non-existent UARTs.  Only later, when the real UART drivers are probed
and their devices are instantiated will the console switch back over to
the proper UART.

This is noticeable when using earlycon, since part of the serial console
log will appear to disappear (when the bogus old takes over) and then
re-appear (when the real UART finally gets registered for the console).

The problem is even more noticable when *not* using earlycon, since in
this case the entire console output is missing, having been incorrectly
played back to the non-existing serial port.

Create a global variable to allow skipping old serial port initialization
and wire it up to the AMDCZ ACPI SPCR quirk and the special amdcz earlycon
setup handler.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
Changes since v1:
 * Rename variable to serial8250_skip_old_ports
 * Also set variable in acpi/spcr. to support no-earlycon case.

 drivers/acpi/spcr.c                  | 2 ++
 drivers/tty/serial/8250/8250_core.c  | 6 ++++++
 drivers/tty/serial/8250/8250_early.c | 1 +
 include/linux/serial_8250.h          | 6 ++++++
 4 files changed, 15 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 52d840d0e05b..f59591283410 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/console.h>
 #include <linux/kernel.h>
+#include <linux/serial_8250.h>
 #include <linux/serial_core.h>
 
 /*
@@ -208,6 +209,7 @@ int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
 	}
 
 	if (amdcz_present(table)) {
+		serial8250_skip_old_ports = true;
 		if (enable_earlycon)
 			uart = "amdcz";
 	}
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..a04da392a251 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -66,6 +66,9 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
 #define SERIAL_PORT_DFNS
 #endif
 
+bool serial8250_skip_old_ports;
+EXPORT_SYMBOL(serial8250_skip_old_ports);
+
 static const struct old_serial_port old_serial_port[] = {
 	SERIAL_PORT_DFNS /* defined in asm/serial.h */
 };
@@ -537,6 +540,9 @@ static void __init serial8250_isa_init_ports(void)
 	if (share_irqs)
 		irqflag = IRQF_SHARED;
 
+	if (serial8250_skip_old_ports)
+		return;
+
 	for (i = 0, up = serial8250_ports;
 	     i < ARRAY_SIZE(old_serial_port) && i < nr_uarts;
 	     i++, up++) {
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index c6bf971a6038..288d2be82990 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -202,6 +202,7 @@ static int __init early_amdcz_setup(struct earlycon_device *dev,
 {
 	struct uart_port *port = &dev->port;
 
+	serial8250_skip_old_ports = true;
 	port->uartclk = 48000000;
 
 	return early_serial8250_setup(dev, opt);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a27ef5f56431..02570edaddd8 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -136,6 +136,12 @@ struct uart_8250_port {
 	struct uart_8250_em485 *em485;
 };
 
+#ifdef CONFIG_SERIAL_8250
+extern bool serial8250_skip_old_ports;
+#else
+static const bool serial8250_skip_old_ports;
+#endif
+
 static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
 {
 	return container_of(up, struct uart_8250_port, port);
-- 
2.16.2.804.g6dcf76e118-goog

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Kees Cook]

On Wed, Mar 14, 2018 at 2:44 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> The old_serial_port global array in 8250_core is supposed to hold an entry
> for each serial port on the system that cannot be discovered via a
> standard enumeration mechanism (aka ACPI/PCI/DTS).  The array is populated
> at compile-time from the value specified in the SERIAL_PORT_DFNS macro.
> This macro is defined in arch/serial.h.
>
> For x86, this macro is currently unconditionally initialized to supply
> four ioport UARTs (0x3F8, 0x2F8, 0x3E8, 0x2E8).
>
> However, not all x86 CPUs have these four ioport UARTs.  For example, the
> UARTs on AMD Carrizo and later are separate memory mapped Designware IP
> blocks.
>
> Fairly early in boot the console_initcall univ8250_console_init iterates
> over this array and installs these old UARTs into the global array
> serial8250_ports.  Further, it attempts to register them for use as
> the console.  In other words, if, for example, the kernel commandline has
> console=ttyS0, the console will be switched over to one of these
> non-existent UARTs.  Only later, when the real UART drivers are probed
> and their devices are instantiated will the console switch back over to
> the proper UART.
>
> This is noticeable when using earlycon, since part of the serial console
> log will appear to disappear (when the bogus old takes over) and then
> re-appear (when the real UART finally gets registered for the console).
>
> The problem is even more noticable when *not* using earlycon, since in
> this case the entire console output is missing, having been incorrectly
> played back to the non-existing serial port.
>
> Create a global variable to allow skipping old serial port initialization
> and wire it up to the AMDCZ ACPI SPCR quirk and the special amdcz earlycon
> setup handler.
>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
> Changes since v1:
>  * Rename variable to serial8250_skip_old_ports
>  * Also set variable in acpi/spcr. to support no-earlycon case.
>
>  drivers/acpi/spcr.c                  | 2 ++
>  drivers/tty/serial/8250/8250_core.c  | 6 ++++++
>  drivers/tty/serial/8250/8250_early.c | 1 +
>  include/linux/serial_8250.h          | 6 ++++++
>  4 files changed, 15 insertions(+)
>
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index 52d840d0e05b..f59591283410 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -14,6 +14,7 @@
>  #include <linux/acpi.h>
>  #include <linux/console.h>
>  #include <linux/kernel.h>
> +#include <linux/serial_8250.h>
>  #include <linux/serial_core.h>
>
>  /*
> @@ -208,6 +209,7 @@ int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
>         }
>
>         if (amdcz_present(table)) {
> +               serial8250_skip_old_ports = true;
>                 if (enable_earlycon)
>                         uart = "amdcz";
>         }

Sorry for being dense. What tree is this against? I can't find mention
of amdcz in Linus's tree nor linux-next.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 9342fc2ee7df..a04da392a251 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -66,6 +66,9 @@ static unsigned int skip_txen_test; /* force skip of txen test at init time */
>  #define SERIAL_PORT_DFNS
>  #endif
>
> +bool serial8250_skip_old_ports;
> +EXPORT_SYMBOL(serial8250_skip_old_ports);
> +
>  static const struct old_serial_port old_serial_port[] = {
>         SERIAL_PORT_DFNS /* defined in asm/serial.h */
>  };
> @@ -537,6 +540,9 @@ static void __init serial8250_isa_init_ports(void)
>         if (share_irqs)
>                 irqflag = IRQF_SHARED;
>
> +       if (serial8250_skip_old_ports)
> +               return;
> +
>         for (i = 0, up = serial8250_ports;
>              i < ARRAY_SIZE(old_serial_port) && i < nr_uarts;
>              i++, up++) {
> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
> index c6bf971a6038..288d2be82990 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -202,6 +202,7 @@ static int __init early_amdcz_setup(struct earlycon_device *dev,
>  {
>         struct uart_port *port = &dev->port;
>
> +       serial8250_skip_old_ports = true;
>         port->uartclk = 48000000;
>
>         return early_serial8250_setup(dev, opt);
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index a27ef5f56431..02570edaddd8 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -136,6 +136,12 @@ struct uart_8250_port {
>         struct uart_8250_em485 *em485;
>  };
>
> +#ifdef CONFIG_SERIAL_8250
> +extern bool serial8250_skip_old_ports;
> +#else
> +static const bool serial8250_skip_old_ports;
> +#endif

Where does serial8250_skip_old_ports get used where CONFIG_SERIAL_8250
isn't defined? (i.e. why is the #ifdef needed here?)

> +
>  static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
>  {
>         return container_of(up, struct uart_8250_port, port);
> --
> 2.16.2.804.g6dcf76e118-goog
>

Otherwise, sure, sounds good. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Kees Cook]

On Wed, Mar 14, 2018 at 3:58 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Mar 14, 2018 at 2:44 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Sorry for being dense. What tree is this against? I can't find mention
> of amdcz in Linus's tree nor linux-next.

As I watched this email send, I noticed the "3/3" in the Subject. ;) I
see the amdcz support now. :P
https://patchwork.kernel.org/project/LKML/list/?submitter=18441

>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index a27ef5f56431..02570edaddd8 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -136,6 +136,12 @@ struct uart_8250_port {
>>         struct uart_8250_em485 *em485;
>>  };
>>
>> +#ifdef CONFIG_SERIAL_8250
>> +extern bool serial8250_skip_old_ports;
>> +#else
>> +static const bool serial8250_skip_old_ports;
>> +#endif
>
> Where does serial8250_skip_old_ports get used where CONFIG_SERIAL_8250
> isn't defined? (i.e. why is the #ifdef needed here?)

This question still stands, though.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Daniel Kurtz]

On Wed, Mar 14, 2018 at 5:00 PM Kees Cook <keescook@chromium.org> wrote:

> On Wed, Mar 14, 2018 at 3:58 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Mar 14, 2018 at 2:44 PM, Daniel Kurtz <djkurtz@chromium.org>
wrote:
> > Sorry for being dense. What tree is this against? I can't find mention
> > of amdcz in Linus's tree nor linux-next.

> As I watched this email send, I noticed the "3/3" in the Subject. ;) I
> see the amdcz support now. :P
> https://patchwork.kernel.org/project/LKML/list/?submitter=18441

> >> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> >> index a27ef5f56431..02570edaddd8 100644
> >> --- a/include/linux/serial_8250.h
> >> +++ b/include/linux/serial_8250.h
> >> @@ -136,6 +136,12 @@ struct uart_8250_port {
> >>         struct uart_8250_em485 *em485;
> >>  };
> >>
> >> +#ifdef CONFIG_SERIAL_8250
> >> +extern bool serial8250_skip_old_ports;
> >> +#else
> >> +static const bool serial8250_skip_old_ports;
> >> +#endif
> >
> > Where does serial8250_skip_old_ports get used where CONFIG_SERIAL_8250
> > isn't defined? (i.e. why is the #ifdef needed here?)

> This question still stands, though.

 From the same patch: https://patchwork.kernel.org/patch/10283641/

if (CONFIG_ACPI_SPCR_TABLE && !CONFIG_SERIAL_8250)

in other words, if serial ports are disabled, but we still want to parse
the APCI_SPCR_TABLE, which "defaults y if X86".
Perhaps that logic should be changed (no need to parse ACPI SPCR table if
we are going to disable serial anyway)?


> -Kees

> --
> Kees Cook
> Pixel Security

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Kees Cook]

On Wed, Mar 14, 2018 at 5:23 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Wed, Mar 14, 2018 at 5:00 PM Kees Cook <keescook@chromium.org> wrote:
>
>> On Wed, Mar 14, 2018 at 3:58 PM, Kees Cook <keescook@chromium.org> wrote:
>> > On Wed, Mar 14, 2018 at 2:44 PM, Daniel Kurtz <djkurtz@chromium.org>
> wrote:
>> > Sorry for being dense. What tree is this against? I can't find mention
>> > of amdcz in Linus's tree nor linux-next.
>
>> As I watched this email send, I noticed the "3/3" in the Subject. ;) I
>> see the amdcz support now. :P
>> https://patchwork.kernel.org/project/LKML/list/?submitter=18441
>
>> >> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> >> index a27ef5f56431..02570edaddd8 100644
>> >> --- a/include/linux/serial_8250.h
>> >> +++ b/include/linux/serial_8250.h
>> >> @@ -136,6 +136,12 @@ struct uart_8250_port {
>> >>         struct uart_8250_em485 *em485;
>> >>  };
>> >>
>> >> +#ifdef CONFIG_SERIAL_8250
>> >> +extern bool serial8250_skip_old_ports;
>> >> +#else
>> >> +static const bool serial8250_skip_old_ports;
>> >> +#endif
>> >
>> > Where does serial8250_skip_old_ports get used where CONFIG_SERIAL_8250
>> > isn't defined? (i.e. why is the #ifdef needed here?)
>
>> This question still stands, though.
>
>  From the same patch: https://patchwork.kernel.org/patch/10283641/
>
> if (CONFIG_ACPI_SPCR_TABLE && !CONFIG_SERIAL_8250)
>
> in other words, if serial ports are disabled, but we still want to parse
> the APCI_SPCR_TABLE, which "defaults y if X86".
> Perhaps that logic should be changed (no need to parse ACPI SPCR table if
> we are going to disable serial anyway)?

But won't this break? "static const bool ..." but the code tries to
set a value but I'd expect the compiler to still yet about it?

I think you could drop the .h #ifdef and use:

if (IS_ENABLED(CONFIG_SERIAL_825) && amdcz_present(...)) {

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[Daniel Kurtz]

On Wed, Mar 14, 2018 at 6:55 PM Kees Cook <keescook@chromium.org> wrote:

> On Wed, Mar 14, 2018 at 5:23 PM, Daniel Kurtz <djkurtz@chromium.org>
wrote:
> > On Wed, Mar 14, 2018 at 5:00 PM Kees Cook <keescook@chromium.org> wrote:
> >
> >> On Wed, Mar 14, 2018 at 3:58 PM, Kees Cook <keescook@chromium.org>
wrote:
> >> > On Wed, Mar 14, 2018 at 2:44 PM, Daniel Kurtz <djkurtz@chromium.org>
> > wrote:
> >> > Sorry for being dense. What tree is this against? I can't find
mention
> >> > of amdcz in Linus's tree nor linux-next.
> >
> >> As I watched this email send, I noticed the "3/3" in the Subject. ;) I
> >> see the amdcz support now. :P
> >> https://patchwork.kernel.org/project/LKML/list/?submitter=18441
> >
> >> >> diff --git a/include/linux/serial_8250.h
b/include/linux/serial_8250.h
> >> >> index a27ef5f56431..02570edaddd8 100644
> >> >> --- a/include/linux/serial_8250.h
> >> >> +++ b/include/linux/serial_8250.h
> >> >> @@ -136,6 +136,12 @@ struct uart_8250_port {
> >> >>         struct uart_8250_em485 *em485;
> >> >>  };
> >> >>
> >> >> +#ifdef CONFIG_SERIAL_8250
> >> >> +extern bool serial8250_skip_old_ports;
> >> >> +#else
> >> >> +static const bool serial8250_skip_old_ports;
> >> >> +#endif
> >> >
> >> > Where does serial8250_skip_old_ports get used where
CONFIG_SERIAL_8250
> >> > isn't defined? (i.e. why is the #ifdef needed here?)
> >
> >> This question still stands, though.
> >
> >  From the same patch: https://patchwork.kernel.org/patch/10283641/
> >
> > if (CONFIG_ACPI_SPCR_TABLE && !CONFIG_SERIAL_8250)
> >
> > in other words, if serial ports are disabled, but we still want to parse
> > the APCI_SPCR_TABLE, which "defaults y if X86".
> > Perhaps that logic should be changed (no need to parse ACPI SPCR table
if
> > we are going to disable serial anyway)?

> But won't this break? "static const bool ..." but the code tries to
> set a value but I'd expect the compiler to still yet about it?

er... yeah.


> I think you could drop the .h #ifdef and use:

> if (IS_ENABLED(CONFIG_SERIAL_825) && amdcz_present(...)) {

oooohhh... nice.  I like it, will change.



> -Kees

> --
> Kees Cook
> Pixel Security

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5712 bytes --]

Hi Daniel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Kurtz/Add-earlycon-support-for-AMD-Carrizo-Stoneyridge/20180318-025754
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//acpi/spcr.c: In function 'acpi_parse_spcr':
>> drivers//acpi/spcr.c:212:29: error: assignment of read-only variable 'serial8250_skip_old_ports'
      serial8250_skip_old_ports = true;
                                ^
--
>> drivers//tty/serial/8250/8250_core.c:69:6: error: conflicting type qualifiers for 'serial8250_skip_old_ports'
    bool serial8250_skip_old_ports;
         ^~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers//tty/serial/8250/8250_core.c:29:0:
   include/linux/serial_8250.h:142:19: note: previous declaration of 'serial8250_skip_old_ports' was here
    static const bool serial8250_skip_old_ports;
                      ^~~~~~~~~~~~~~~~~~~~~~~~~

sparse warnings: (new ones prefixed by >>)

>> drivers/acpi/spcr.c:212:17: sparse: assignment to const expression
   drivers/acpi/spcr.c: In function 'acpi_parse_spcr':
   drivers/acpi/spcr.c:212:29: error: assignment of read-only variable 'serial8250_skip_old_ports'
      serial8250_skip_old_ports = true;
                                ^

vim +/serial8250_skip_old_ports +212 drivers//acpi/spcr.c

    94	
    95	/**
    96	 * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
    97	 *
    98	 * @enable_earlycon: set up earlycon for the console specified by the table
    99	 * @enable_console: setup the console specified by the table.
   100	 *
   101	 * For the architectures with support for ACPI, CONFIG_ACPI_SPCR_TABLE may be
   102	 * defined to parse ACPI SPCR table.  As a result of the parsing preferred
   103	 * console is registered and if @enable_earlycon is true, earlycon is set up.
   104	 * If @enable_console is true the system console is also configured.
   105	 *
   106	 * When CONFIG_ACPI_SPCR_TABLE is defined, this function should be called
   107	 * from arch initialization code as soon as the DT/ACPI decision is made.
   108	 *
   109	 */
   110	int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
   111	{
   112		static char opts[64];
   113		struct acpi_table_spcr *table;
   114		acpi_status status;
   115		char *uart;
   116		char *iotype;
   117		int baud_rate;
   118		int err;
   119	
   120		if (acpi_disabled)
   121			return -ENODEV;
   122	
   123		status = acpi_get_table(ACPI_SIG_SPCR, 0,
   124					(struct acpi_table_header **)&table);
   125	
   126		if (ACPI_FAILURE(status))
   127			return -ENOENT;
   128	
   129		if (table->header.revision < 2)
   130			pr_info("SPCR table version %d\n", table->header.revision);
   131	
   132		if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
   133			switch (ACPI_ACCESS_BIT_WIDTH((
   134				table->serial_port.access_width))) {
   135			default:
   136				pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
   137				/* fall through */
   138			case 8:
   139				iotype = "mmio";
   140				break;
   141			case 16:
   142				iotype = "mmio16";
   143				break;
   144			case 32:
   145				iotype = "mmio32";
   146				break;
   147			}
   148		} else
   149			iotype = "io";
   150	
   151		switch (table->interface_type) {
   152		case ACPI_DBG2_ARM_SBSA_32BIT:
   153			iotype = "mmio32";
   154			/* fall through */
   155		case ACPI_DBG2_ARM_PL011:
   156		case ACPI_DBG2_ARM_SBSA_GENERIC:
   157		case ACPI_DBG2_BCM2835:
   158			uart = "pl011";
   159			break;
   160		case ACPI_DBG2_16550_COMPATIBLE:
   161		case ACPI_DBG2_16550_SUBSET:
   162			uart = "uart";
   163			break;
   164		default:
   165			err = -ENOENT;
   166			goto done;
   167		}
   168	
   169		switch (table->baud_rate) {
   170		case 3:
   171			baud_rate = 9600;
   172			break;
   173		case 4:
   174			baud_rate = 19200;
   175			break;
   176		case 6:
   177			baud_rate = 57600;
   178			break;
   179		case 7:
   180			baud_rate = 115200;
   181			break;
   182		default:
   183			err = -ENOENT;
   184			goto done;
   185		}
   186	
   187		/*
   188		 * If the E44 erratum is required, then we need to tell the pl011
   189		 * driver to implement the work-around.
   190		 *
   191		 * The global variable is used by the probe function when it
   192		 * creates the UARTs, whether or not they're used as a console.
   193		 *
   194		 * If the user specifies "traditional" earlycon, the qdf2400_e44
   195		 * console name matches the EARLYCON_DECLARE() statement, and
   196		 * SPCR is not used.  Parameter "earlycon" is false.
   197		 *
   198		 * If the user specifies "SPCR" earlycon, then we need to update
   199		 * the console name so that it also says "qdf2400_e44".  Parameter
   200		 * "earlycon" is true.
   201		 *
   202		 * For consistency, if we change the console name, then we do it
   203		 * for everyone, not just earlycon.
   204		 */
   205		if (qdf2400_erratum_44_present(&table->header)) {
   206			qdf2400_e44_present = true;
   207			if (enable_earlycon)
   208				uart = "qdf2400_e44";
   209		}
   210	
   211		if (amdcz_present(table)) {
 > 212			serial8250_skip_old_ports = true;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63094 bytes --]

Re: [PATCH v2 3/3] serial: core: Allow skipping old serial port initialization

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5712 bytes --]

Hi Daniel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Kurtz/Add-earlycon-support-for-AMD-Carrizo-Stoneyridge/20180318-025754
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//acpi/spcr.c: In function 'acpi_parse_spcr':
>> drivers//acpi/spcr.c:212:29: error: assignment of read-only variable 'serial8250_skip_old_ports'
      serial8250_skip_old_ports = true;
                                ^
--
>> drivers//tty/serial/8250/8250_core.c:69:6: error: conflicting type qualifiers for 'serial8250_skip_old_ports'
    bool serial8250_skip_old_ports;
         ^~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers//tty/serial/8250/8250_core.c:29:0:
   include/linux/serial_8250.h:142:19: note: previous declaration of 'serial8250_skip_old_ports' was here
    static const bool serial8250_skip_old_ports;
                      ^~~~~~~~~~~~~~~~~~~~~~~~~

sparse warnings: (new ones prefixed by >>)

>> drivers/acpi/spcr.c:212:17: sparse: assignment to const expression
   drivers/acpi/spcr.c: In function 'acpi_parse_spcr':
   drivers/acpi/spcr.c:212:29: error: assignment of read-only variable 'serial8250_skip_old_ports'
      serial8250_skip_old_ports = true;
                                ^

vim +/serial8250_skip_old_ports +212 drivers//acpi/spcr.c

    94	
    95	/**
    96	 * acpi_parse_spcr() - parse ACPI SPCR table and add preferred console
    97	 *
    98	 * @enable_earlycon: set up earlycon for the console specified by the table
    99	 * @enable_console: setup the console specified by the table.
   100	 *
   101	 * For the architectures with support for ACPI, CONFIG_ACPI_SPCR_TABLE may be
   102	 * defined to parse ACPI SPCR table.  As a result of the parsing preferred
   103	 * console is registered and if @enable_earlycon is true, earlycon is set up.
   104	 * If @enable_console is true the system console is also configured.
   105	 *
   106	 * When CONFIG_ACPI_SPCR_TABLE is defined, this function should be called
   107	 * from arch initialization code as soon as the DT/ACPI decision is made.
   108	 *
   109	 */
   110	int __init acpi_parse_spcr(bool enable_earlycon, bool enable_console)
   111	{
   112		static char opts[64];
   113		struct acpi_table_spcr *table;
   114		acpi_status status;
   115		char *uart;
   116		char *iotype;
   117		int baud_rate;
   118		int err;
   119	
   120		if (acpi_disabled)
   121			return -ENODEV;
   122	
   123		status = acpi_get_table(ACPI_SIG_SPCR, 0,
   124					(struct acpi_table_header **)&table);
   125	
   126		if (ACPI_FAILURE(status))
   127			return -ENOENT;
   128	
   129		if (table->header.revision < 2)
   130			pr_info("SPCR table version %d\n", table->header.revision);
   131	
   132		if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
   133			switch (ACPI_ACCESS_BIT_WIDTH((
   134				table->serial_port.access_width))) {
   135			default:
   136				pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
   137				/* fall through */
   138			case 8:
   139				iotype = "mmio";
   140				break;
   141			case 16:
   142				iotype = "mmio16";
   143				break;
   144			case 32:
   145				iotype = "mmio32";
   146				break;
   147			}
   148		} else
   149			iotype = "io";
   150	
   151		switch (table->interface_type) {
   152		case ACPI_DBG2_ARM_SBSA_32BIT:
   153			iotype = "mmio32";
   154			/* fall through */
   155		case ACPI_DBG2_ARM_PL011:
   156		case ACPI_DBG2_ARM_SBSA_GENERIC:
   157		case ACPI_DBG2_BCM2835:
   158			uart = "pl011";
   159			break;
   160		case ACPI_DBG2_16550_COMPATIBLE:
   161		case ACPI_DBG2_16550_SUBSET:
   162			uart = "uart";
   163			break;
   164		default:
   165			err = -ENOENT;
   166			goto done;
   167		}
   168	
   169		switch (table->baud_rate) {
   170		case 3:
   171			baud_rate = 9600;
   172			break;
   173		case 4:
   174			baud_rate = 19200;
   175			break;
   176		case 6:
   177			baud_rate = 57600;
   178			break;
   179		case 7:
   180			baud_rate = 115200;
   181			break;
   182		default:
   183			err = -ENOENT;
   184			goto done;
   185		}
   186	
   187		/*
   188		 * If the E44 erratum is required, then we need to tell the pl011
   189		 * driver to implement the work-around.
   190		 *
   191		 * The global variable is used by the probe function when it
   192		 * creates the UARTs, whether or not they're used as a console.
   193		 *
   194		 * If the user specifies "traditional" earlycon, the qdf2400_e44
   195		 * console name matches the EARLYCON_DECLARE() statement, and
   196		 * SPCR is not used.  Parameter "earlycon" is false.
   197		 *
   198		 * If the user specifies "SPCR" earlycon, then we need to update
   199		 * the console name so that it also says "qdf2400_e44".  Parameter
   200		 * "earlycon" is true.
   201		 *
   202		 * For consistency, if we change the console name, then we do it
   203		 * for everyone, not just earlycon.
   204		 */
   205		if (qdf2400_erratum_44_present(&table->header)) {
   206			qdf2400_e44_present = true;
   207			if (enable_earlycon)
   208				uart = "qdf2400_e44";
   209		}
   210	
   211		if (amdcz_present(table)) {
 > 212			serial8250_skip_old_ports = true;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63094 bytes --]

Re: [PATCH v2 2/3] ACPI: SPCR: Add support for AMD CT/SZ

[Rafael J. Wysocki]

On Wednesday, March 14, 2018 10:44:37 PM CET Daniel Kurtz wrote:
> AMD Carrizo / Stoneyridge use a DesignWare 8250 UART that uses a special
> earlycon setup handler to configure its input clock in order to compute
> baud rate divisor registers.
> Detect them by examining the OEMID field in the SPCR header, and pass
> then pass uart type amdcz to earlycon.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Can you possibly send the entire series again and CC all patches to linux-acpi
and fix the kbuild warnings if the are relevant for that matter?

Thanks!