serial_core: add pci uart early console support

On some Intel Atom SoCs, the legacy IO port UART(0x3F8) is not available. Instead, a 8250 compatible PCI uart can be used as early console. This patch adds pci support to the 8250 early console driver uart8250. For example, to enable pci uart(00:21.3) as early console on these platforms, append the following line to the kernel command line (assume baud rate is 115200):
earlyprintk=uart8250,pci32,0:24.2,115200n8

Signed-off-by Bin Gao <[email protected]>

---
Changes in v4:
- moved PCI_EARLY definition from arch/x86/Kconfig to drivers/pci/Kconfig
- added 'earlyprintk' for x86 as alias to the early param 'earlycon'

Quoted 5 lines

Signed-off-by Bin Gao <[email protected]>

---
Changes in v4:
- moved PCI_EARLY definition from arch/x86/Kconfig to drivers/pci/Kconfig
- added 'earlyprintk' for x86 as alias to the early param 'earlycon'

What about the changes in earlier versions?

And why do I have 3 copies of different "v4" patches in my inbox?

Please resend this, as v5, correctly, and we can review it that way.

greg k-h

On some Intel Atom SoCs, the legacy IO port UART(0x3F8) is not available. Instead, a 8250 compatible PCI uart can be used as early console. This patch adds pci support to the 8250 early console driver uart8250. For example, to enable pci uart(00:21.3) as early console on these platforms, append the following line to the kernel command line (assume baud rate is 115200):
earlyprintk=uart8250,pci32,0:24.2,115200n8

Signed-off-by Bin Gao <[email protected]>

---
Changes in v4:
- moved PCI_EARLY definition from arch/x86/Kconfig to drivers/pci/Kconfig
- added 'earlyprintk' for x86 as alias to the early param 'earlycon'

 arch/x86/Kconfig                 |   1 +
 drivers/pci/Kconfig              |  11 +++
 drivers/tty/serial/earlycon.c    |   9 +++
 drivers/tty/serial/serial_core.c | 145 ++++++++++++++++++++++++++++++++++++++-

 4 files changed, 164 insertions(+), 2 deletions(-)

arch/x86/Kconfig +1 -0

index 226d569..bdedd61 100644
@@ -143,6 +143,7 @@ config X86
 	select ACPI_LEGACY_TABLES_LOOKUP if ACPI
 	select X86_FEATURE_NAMES if PROC_FS
 	select SRCU
+	select PCI_EARLY if PCI
 
 config INSTRUCTION_DECODER
 	def_bool y

drivers/pci/Kconfig +11 -0

index 7a8f1c5..4f0f055 100644
@@ -114,4 +114,15 @@ config PCI_LABEL
 	def_bool y if (DMI || ACPI)
 	select NLS
 
+config PCI_EARLY
+	bool "Early PCI access"
+	depends on PCI
+	default n
+	help
+	  This option indicates that a group of APIs are available (in
+	  asm/pci-direct.h) so the kernel can access pci config registers
+	  before the PCI subsystem is initialized. Any arch that supports
+	  early pci APIs should enable this option which is required  by
+	  arch independent codes, e.g. uart8250 pci early console driver.
+
 source "drivers/pci/host/Kconfig"

drivers/tty/serial/earlycon.c +0 -0

index 6dc471e..63ae60e 100644
@@ -193,6 +193,15 @@ static int __init param_setup_earlycon(char *buf)
 }
 early_param("earlycon", param_setup_earlycon);

drivers/tty/serial/earlycon.c +0 -0

 int __init of_setup_earlycon(unsigned long addr,
 			     int (*setup)(struct earlycon_device *, const char *))
 {

drivers/tty/serial/serial_core.c +5 -0

index 0b7bb12..19ca2a0 100644
@@ -34,10 +34,15 @@
 #include <linux/serial_core.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
+#include <linux/pci_regs.h>
 
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_PCI_EARLY
+#include <asm/pci-direct.h>
+#endif
+
 /*
  * This is used to lock changes in serial line configuration.
  */

drivers/tty/serial/serial_core.c +8 -0

+{
+	char str[4]; /* max 3 chars, plus a NULL terminator */
+	char *p = options;
+	int i = 0;
+
+	while (*p) {
+		if (i >= 4)
+			return -EINVAL;

drivers/tty/serial/serial_core.c +21 -0

+
+	return -EINVAL;
+}
+
+#ifdef CONFIG_PCI_EARLY
+/*
+ * The whole pci option from the command line is: pci[32],B:D.F[,options]
+ * Examples:
+ *     pci,0:21.3,115200n8
+ *     pci32,0:21.3
+ * Here pci32 means 8250 UART registers are 32-bit width(regshift = 2).
+ * pci means 8250 UART registers are 8-bit width(regshift = 0).
+ * B,D and F are bus, device and function, in decimal(not hex).
+ * The additional options(115200n8) would be parsed by the earlycon framework.
+ *
+ * @options: the pci options
+ * @phys: the pointer to return pci mem or io address
+ * return: <0: error
+ *          0: pci mem
+ *          1: pci io
+ */

drivers/tty/serial/serial_core.c +5 -0

+		return -EINVAL;
+	}
+
+	/* We come here with options=B:D.F[,options] */
+	if (parse_bdf(options, &endp, ':', &bus))

drivers/tty/serial/serial_core.c +14 -0

+	 * so skip checking it.
+	 */
+
+	bar0 = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
+
+	/* The BAR is IO or Memory? */
+	if ((bar0 & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO)
+		pci_io = 1;
+
+	if ((bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+			PCI_BASE_ADDRESS_MEM_TYPE_64)
+		bar0 |= (u64)read_pci_config(bus, dev, func,
+				PCI_BASE_ADDRESS_0 + 4) << 32;
+

drivers/tty/serial/serial_core.c +23 -1

  *	   console=<name>,io|mmio|mmio32,<addr>,<options>
+ *	For pci/pci32, the <addr> format is B:D.F, e.g. 0:24.2
  *
  *	The optional form
  *	   earlycon=<name>,0x<addr>,<options>

@@ -1829,12 +1939,23 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
 int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 			char **options)
 {
+	int pci = 0, ret;
+	unsigned long phys;
+
 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
+	} else if (strncmp(p, "pci,", 4) == 0) {
+		pci = 1;
+		p += 4;
+		ret = parse_pci_options(p, &phys);
+	} else if (strncmp(p, "pci32,", 6) == 0) {
+		pci = 2;
+		p += 6;
+		ret = parse_pci_options(p, &phys);
 	} else if (strncmp(p, "io,", 3) == 0) {
 		*iotype = UPIO_PORT;
 		p += 3;

@@ -1844,7 +1965,27 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 		return -EINVAL;
 	}
 
-	*addr = simple_strtoul(p, NULL, 0);
+	if (pci) {
+		if (ret < 0) /* error */
+			return ret;
+
+		/*
+		 * Once PCI mem/io is read from PCI BAR, we can reuse
+		 * mmio/mmio32/io type to minimize code change.
+		 */
+		if (ret > 0) /* PCI io */
+			*iotype = UPIO_PORT;
+		else { /* ret = 0: PCI mem */

drivers/tty/serial/serial_core.c +0 -1

-	*addr = simple_strtoul(p, NULL, 0);
 	p = strchr(p, ',');
 	if (p)
 		p++;

Hi Bin,

Please don't drop lists (or other addressees) from patch revisions.

[ +cc linux-serial]

Please update Documentation/kernel-parameters.txt with pci-specific earlycon parameter formats.

Please move this hunk to patch 2/2.

FYI, there is a proposal to evaluate "earlyprintk=" earlier than parse_early_params(), which this would break.

https://lkml.org/lkml/2015/5/18/127

Quoted 1 line

+static int parse_bdf(char *options, char **endp, char delimiter, u8 *val)

If this function is only used from parse_pci_options(), please enclose it in #ifdef CONFIG_PCI_EARLY scope with parse_pci_options().

Quoted 15 lines

drivers/tty/serial/serial_core.c +14 -0

+{
+	char str[4]; /* max 3 chars, plus a NULL terminator */
+	char *p = options;
+	int i = 0;
+
+	while (*p) {
+		if (i >= 4)
+			return -EINVAL;
+
+		if (*p == delimiter) {
+			str[i++] = 0;
+			if (endp)
+				*endp = p + 1;
+			return kstrtou8(str, 10, val); /* decimal, no hex */

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1834

Quoted 4 lines

drivers/tty/serial/serial_core.c +4 -0

+		}
+
+		str[i++] = *p++;
+	}

Is all this to avoid using simple_strtoul()?
If yes, I'd rather you use simple_strtoul() like the rest of the console code and ignore the (misguided) advice that simple_strtoul() is obsolete.

The code above is exactly what is wrong with the kstrto* api.

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1865

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	if (!early_pci_allowed()) {
+		pr_err("earlycon pci not available(early pci not allowed)\n");

Error message is redundant.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/*
+	 * On these platforms class code in pci config is broken,

              ^^^^^^^^^^^^^^^

which platforms?

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1904

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	pr_info("Use 8250 uart at PCI 0000:%02u:%02u.%01u as early console\n",
+							bus, dev, func);

I think one earlycon banner is sufficient; you could make this pr_debug() instead. Existing convention for earlycon messages is

	"earlycon: ...."

Quoted 5 lines

* @options: ptr for <options> field; NULL if not present (out)
*
* Decodes earlycon kernel command line parameters of the form

- *	   earlycon=<name>,io|mmio|mmio32,<addr>,<options>
+ *	   earlycon=<name>,io|mmio|mmio32|pci|pci32,<addr>,<options>

Document the pci/pci32 format separately because
earlycon=uart8250,pci32,<addr>,<options> is not a valid form.

Quoted 40 lines

drivers/tty/serial/serial_core.c +22 -1

@@ -1829,12 +1939,23 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
 int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 			char **options)
 {
+	int pci = 0, ret;
+	unsigned long phys;
+
 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
+	} else if (strncmp(p, "pci,", 4) == 0) {
+		pci = 1;
+		p += 4;
+		ret = parse_pci_options(p, &phys);
+	} else if (strncmp(p, "pci32,", 6) == 0) {
+		pci = 2;
+		p += 6;
+		ret = parse_pci_options(p, &phys);
 	} else if (strncmp(p, "io,", 3) == 0) {
 		*iotype = UPIO_PORT;
 		p += 3;

@@ -1844,7 +1965,27 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 		return -EINVAL;
 	}
 
-	*addr = simple_strtoul(p, NULL, 0);
+	if (pci) {
+		if (ret < 0) /* error */
+			return ret;
+
+		/*
+		 * Once PCI mem/io is read from PCI BAR, we can reuse
+		 * mmio/mmio32/io type to minimize code change.
+		 */
+		if (ret > 0) /* PCI io */
+			*iotype = UPIO_PORT;
+		else { /* ret = 0: PCI mem */

drivers/tty/serial/serial_core.c int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, · line 1988

Quoted 10 lines

drivers/tty/serial/serial_core.c +10 -0

+			if (pci == 2)
+				*iotype = UPIO_MEM32;
+			else
+				*iotype = UPIO_MEM;
+		}
+
+		*addr = phys;
+	} else
+		*addr = simple_strtoul(p, NULL, 0);
+

I'd like to see this refactored without the logic steering locals. Like this:

drivers/tty/serial/serial_core.c +17 -0

 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
+		*addr = simple_strtoul(p, NULL, 0);
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
+		*addr = simple_strtoul(p, NULL, 0);
+	} else if (strncmp(p, "pci,", 4) == 0) {
+		pci = 1;
+		p += 4;
+		ret = parse_pci_options(p, addr);
+		if (ret < 0)
+			return ret;
+		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM;
+	} else if (strncmp(p, "pci32,", 6) == 0) {
+		pci = 2;
+		p += 6;
+		ret = parse_pci_options(p, addr);
+		if (ret < 0)
+			return ret;
+		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM32;
 	} else if (strncmp(p, "io,", 3) == 0) {
 		*iotype = UPIO_PORT;
 		p += 3;
+		*addr = simple_strtoul(p, NULL, 0);

Regards,
Peter Hurley

drivers/tty/serial/serial_core.c int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, · line 1991

Quoted 5 lines

drivers/tty/serial/serial_core.c +0 -1

-	*addr = simple_strtoul(p, NULL, 0);
 	p = strchr(p, ',');
 	if (p)
 		p++;

Peter Hurley,
First of all, thank you for your reviewing.
Please see my answers below.

Quoted 5 lines · Peter Hurley

Hi Bin,

Please don't drop lists (or other addressees) from patch revisions.

[ +cc linux-serial]

Will fix this.

Quoted 2 lines

Please update Documentation/kernel-parameters.txt with pci-specific earlycon parameter formats.

Will do.

Quoted 1 line

Please move this hunk to patch 2/2.

It's already in 2/2. Or I'm not quite understanding?

drivers/tty/serial/earlycon.c +9 -0

+/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
+#ifdef CONFIG_X86
+static int __init param_setup_earlycon_x86(char *buf)
+{
+	return param_setup_earlycon(buf);
+}
+early_param("earlyprintk", param_setup_earlycon_x86);
+#endif
+

Quoted 4 lines

FYI, there is a proposal to evaluate "earlyprintk=" earlier than parse_early_params(), which this would break.

https://lkml.org/lkml/2015/5/18/127

No, it wouldn't. The proposal only moves earlyprintk=serial(legacy 0x3f8). If you look into arch/x86/kernel/early_printk.c, you'll find many other options other than serial, e.g. vga, dbgp, xen, efi, pciserial, etc. uart8250(previously pciserial) is one of these "others".

Quoted 4 lines

Quoted 1 line

+static int parse_bdf(char *options, char **endp, char delimiter, u8 *val)

If this function is only used from parse_pci_options(), please enclose it in #ifdef CONFIG_PCI_EARLY scope with parse_pci_options().

Yes, will fix it.

Quoted 15 lines

Quoted 15 lines

drivers/tty/serial/serial_core.c +14 -0

+{
+	char str[4]; /* max 3 chars, plus a NULL terminator */
+	char *p = options;
+	int i = 0;
+
+	while (*p) {
+		if (i >= 4)
+			return -EINVAL;
+
+		if (*p == delimiter) {
+			str[i++] = 0;
+			if (endp)
+				*endp = p + 1;
+			return kstrtou8(str, 10, val); /* decimal, no hex */

Quoted 8 lines

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1834

Quoted 4 lines

drivers/tty/serial/serial_core.c +4 -0

+		}
+
+		str[i++] = *p++;
+	}

Is all this to avoid using simple_strtoul()?
If yes, I'd rather you use simple_strtoul() like the rest of the console code and ignore the (misguided) advice that simple_strtoul() is obsolete.

Then scripts/checkpatch.pl should be updated to avoid this kind of "misguiding". Also the kstrto* API descriptions should be updated...

Quoted 1 line

The code above is exactly what is wrong with the kstrto* api.

Can you elaborate a little bit? Thanks.

Quoted 4 lines

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1865

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	if (!early_pci_allowed()) {
+		pr_err("earlycon pci not available(early pci not allowed)\n");

Error message is redundant.

"early pci not allowed" is the reason of "earlycon pci not available".

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/*
+	 * On these platforms class code in pci config is broken,

              ^^^^^^^^^^^^^^^

which platforms?

On platforms where this code will run on.
Do you prefer something like:
On platforms with early pci serial class code in pci config is broken,

Quoted 7 lines

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1904

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	pr_info("Use 8250 uart at PCI 0000:%02u:%02u.%01u as early console\n",
+							bus, dev, func);

I think one earlycon banner is sufficient; you could make this pr_debug() instead. Existing convention for earlycon messages is

	"earlycon: ...."

Will use pr_debug().

Quoted 6 lines

Quoted 5 lines

* @options: ptr for <options> field; NULL if not present (out)
*
* Decodes earlycon kernel command line parameters of the form

- *	   earlycon=<name>,io|mmio|mmio32,<addr>,<options>
+ *	   earlycon=<name>,io|mmio|mmio32|pci|pci32,<addr>,<options>

Quoted 2 lines

Document the pci/pci32 format separately because
earlycon=uart8250,pci32,<addr>,<options> is not a valid form.

Why it's not a valid form? It follows the existed syntax like this:
earlycon=uart8250,<io type>,<addr>,<options>

Quoted 85 lines

Quoted 40 lines

drivers/tty/serial/serial_core.c +22 -1

@@ -1829,12 +1939,23 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
 int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 			char **options)
 {
+	int pci = 0, ret;
+	unsigned long phys;
+
 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
+	} else if (strncmp(p, "pci,", 4) == 0) {
+		pci = 1;
+		p += 4;
+		ret = parse_pci_options(p, &phys);
+	} else if (strncmp(p, "pci32,", 6) == 0) {
+		pci = 2;
+		p += 6;
+		ret = parse_pci_options(p, &phys);
 	} else if (strncmp(p, "io,", 3) == 0) {
 		*iotype = UPIO_PORT;
 		p += 3;

@@ -1844,7 +1965,27 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
 		return -EINVAL;
 	}
 
-	*addr = simple_strtoul(p, NULL, 0);
+	if (pci) {
+		if (ret < 0) /* error */
+			return ret;
+
+		/*
+		 * Once PCI mem/io is read from PCI BAR, we can reuse
+		 * mmio/mmio32/io type to minimize code change.
+		 */
+		if (ret > 0) /* PCI io */
+			*iotype = UPIO_PORT;
+		else { /* ret = 0: PCI mem */

drivers/tty/serial/serial_core.c int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, · line 1988

Quoted 10 lines

drivers/tty/serial/serial_core.c +10 -0

+			if (pci == 2)
+				*iotype = UPIO_MEM32;
+			else
+				*iotype = UPIO_MEM;
+		}
+
+		*addr = phys;
+	} else
+		*addr = simple_strtoul(p, NULL, 0);
+

I'd like to see this refactored without the logic steering locals. Like this:

drivers/tty/serial/serial_core.c +17 -0

 	if (strncmp(p, "mmio,", 5) == 0) {
 		*iotype = UPIO_MEM;
 		p += 5;
+		*addr = simple_strtoul(p, NULL, 0);
 	} else if (strncmp(p, "mmio32,", 7) == 0) {
 		*iotype = UPIO_MEM32;
 		p += 7;
+		*addr = simple_strtoul(p, NULL, 0);
+	} else if (strncmp(p, "pci,", 4) == 0) {
+		pci = 1;
+		p += 4;
+		ret = parse_pci_options(p, addr);
+		if (ret < 0)
+			return ret;
+		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM;
+	} else if (strncmp(p, "pci32,", 6) == 0) {
+		pci = 2;
+		p += 6;
+		ret = parse_pci_options(p, addr);
+		if (ret < 0)
+			return ret;
+		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM32;
 	} else if (strncmp(p, "io,", 3) == 0) {
 		*iotype = UPIO_PORT;
 		p += 3;
+		*addr = simple_strtoul(p, NULL, 0);

Regards,
Peter Hurley

Quoted 5 lines

drivers/tty/serial/serial_core.c int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr, · line 1991

Quoted 5 lines

drivers/tty/serial/serial_core.c +0 -1

-	*addr = simple_strtoul(p, NULL, 0);
 	p = strchr(p, ',');
 	if (p)
 		p++;

Ah, this is kind of coding style. But I agree with your suggestion. Will fix it.

I meant that the patch hunk below should be moved to patch 2/2, and the purpose of patch 2/2 should be to replace x86-specific earlyprintk=pciserial with arch-independent earlyprintk=pciserial.

There are 2 reasons for my suggestion:
1) Changes to x86 earlyprintk should get acks from the x86 maintainers. Borislav Petkov <[email protected]> is particularly involved in the 'early' earlyprintk proposal. Patch 2/2 should cc them and the author of the original earlyprintk=pciserial patch.
2) Changes to x86 earlyprintk may be rejected.

drivers/tty/serial/earlycon.c static int __init param_setup_earlycon(char *buf) · line 204

Quoted 9 lines

drivers/tty/serial/earlycon.c +9 -0

+/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
+#ifdef CONFIG_X86
+static int __init param_setup_earlycon_x86(char *buf)
+{
+	return param_setup_earlycon(buf);
+}
+early_param("earlyprintk", param_setup_earlycon_x86);
+#endif
+

Quoted 8 lines

Quoted 4 lines

FYI, there is a proposal to evaluate "earlyprintk=" earlier than parse_early_params(), which this would break.

https://lkml.org/lkml/2015/5/18/127

No, it wouldn't. The proposal only moves earlyprintk=serial(legacy 0x3f8). If you look into arch/x86/kernel/early_printk.c, you'll find many other options other than serial, e.g. vga, dbgp, xen, efi, pciserial, etc. uart8250(previously pciserial) is one of these "others".

I see; so when re-evaluating earlyprintk= for earlycon, there is no danger of corrupting the device state for earlyprintk?

However, the namespace will now be shared so that is an important change that maintainers and future earlycon/earlyprintk authors need to know.

Quoted 2 lines

Then scripts/checkpatch.pl should be updated to avoid this kind of "misguiding". Also the kstrto* API descriptions should be updated...

I know; I've already made that point to Joe.

Quoted 2 lines

Quoted 1 line

The code above is exactly what is wrong with the kstrto* api.

Can you elaborate a little bit? Thanks.

I don't mean there is anything wrong with _your_ code, per se. Rather that it should not be necessary to write 14 lines of code, use temp storage and visit the entire string three times simply to parse a string into a number. IOW, the kstrto* API usefulness is limited, which is why simple_strto* is _not_ obsolete.

Quoted 4 lines

Quoted 4 lines

drivers/tty/serial/serial_core.c uart_get_console(struct uart_port *ports, int nr, struct console *co) · line 1865

Quoted 2 lines

drivers/tty/serial/serial_core.c +2 -0

+	if (!early_pci_allowed()) {
+		pr_err("earlycon pci not available(early pci not allowed)\n");

Error message is redundant.

Quoted 1 line

"early pci not allowed" is the reason of "earlycon pci not available".

Only one or the other is necessary. For example,

"earlycon: early pci not allowed\n"

Quoted 6 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+	/*
+	 * On these platforms class code in pci config is broken,

              ^^^^^^^^^^^^^^^

which platforms?

Quoted 3 lines

On platforms where this code will run on.
Do you prefer something like:
On platforms with early pci serial class code in pci config is broken,

This statement doesn't make sense to me: the original earlyprintk=pciserial implementation read the class code register, so it worked then.

Why not now?

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

* @options: ptr for <options> field; NULL if not present (out)
*
* Decodes earlycon kernel command line parameters of the form

- *	   earlycon=<name>,io|mmio|mmio32,<addr>,<options>
+ *	   earlycon=<name>,io|mmio|mmio32|pci|pci32,<addr>,<options>

Document the pci/pci32 format separately because
earlycon=uart8250,pci32,<addr>,<options> is not a valid form.

Why it's not a valid form? It follows the existed syntax like this:
earlycon=uart8250,<io type>,<addr>,<options>

Well, isn't <addr> being retrieved from BAR0? Why would you specify the <addr> on the command line?

But I see now that's where the 'bus:device.function' goes. I think if I was confused, others will be too. Further, in the context of this code 'addr' is a variable to which the command line parameter comment identifies, whereas 'bus:device.function' does not follow the same convention.