usb: early: add support for early printk through USB3 debug port

xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. Software learns this capability by walking through the extended capability list of the host. xHCI specification describes DbC in section 7.6.

This patch introduces the code to probe and initialize the debug capability hardware during early boot. With hardware initialized, the debug target (system on which this code is running) will present a debug device through the debug port (normally the first USB3 port). The debug device is fully compliant with the USB framework and provides the equivalent of a very high performance (USB3) full-duplex serial link between the debug host and target. The DbC functionality is independent of xHCI host. There isn't any precondition from xHCI host side for DbC to work.

This patch also includes bulk out and bulk in interfaces. These interfaces could be used to implement early printk bootconsole or hook to various system debuggers.

arch/x86/Kconfig.debug +11 -0

+	depends on EARLY_PRINTK && PCI
+	select EARLY_PRINTK_USB
+	---help---
+	  Write kernel log output directly into the xHCI debug port.
+
+	  This is useful for kernel debugging when your machine crashes very
+	  early before the console code is initialized. For normal operation
+	  it is not recommended because it looks ugly and doesn't cooperate
+	  with klogd/syslogd or the X server. You should normally N here,
+	  unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
 	def_bool n

drivers/usb/Kconfig +3 -0

index fbe493d..9313fff 100644
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
 config USB_EHCI_BIG_ENDIAN_DESC
 	bool
 
+config USB_EARLY_PRINTK
+	bool
+
 menuconfig USB_SUPPORT
 	bool "USB support"
 	depends on HAS_IOMEM

drivers/usb/Makefile +1 -1

index 7791af6..53d1356 100644
@@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)	+= image/
 obj-$(CONFIG_USB_SERIAL)	+= serial/
 
 obj-$(CONFIG_USB)		+= misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
+obj-$(CONFIG_EARLY_PRINTK_USB)	+= early/
 
 obj-$(CONFIG_USB_ATM)		+= atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)	+= atm/

drivers/usb/early/Makefile +1 -0

index 24bbe51..fcde228 100644
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_USB_XDBC) += xhci-dbc.o

drivers/usb/early/xhci-dbc.c new file +50 -0

new file mode 100644
index 0000000..72480c4
@@ -0,0 +1,1027 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * 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)	KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include <linux/console.h>
+#include <linux/pci_regs.h>
+#include <linux/pci_ids.h>
+#include <linux/bootmem.h>
+#include <linux/io.h>
+#include <asm/pci-direct.h>
+#include <asm/fixmap.h>
+#include <linux/bcd.h>
+#include <linux/export.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int early_console_keep;
+
+#ifdef XDBC_TRACE
+#define	xdbc_trace	trace_printk
+#else
+static inline void xdbc_trace(const char *fmt, ...) { }
+#endif /* XDBC_TRACE */
+
+static int xdbc_bulk_transfer(void *data, int size, bool read);
+
+static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
+{
+	u32 val, sz;
+	u64 val64, sz64, mask64;
+	u8 byte;
+	void __iomem *base;
+
+	val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
+	write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);

drivers/usb/early/xhci-dbc.c new file +12 -0

+static u32 __init xdbc_find_dbgp(int xdbc_num, u32 *b, u32 *d, u32 *f)
+{
+	u32 bus, dev, func, class;
+
+	for (bus = 0; bus < XDBC_PCI_MAX_BUSES; bus++) {
+		for (dev = 0; dev < XDBC_PCI_MAX_DEVICES; dev++) {
+			for (func = 0; func < XDBC_PCI_MAX_FUNCTION; func++) {
+				class = read_pci_config(bus, dev, func, PCI_CLASS_REVISION);
+				if ((class >> 8) != PCI_CLASS_SERIAL_USB_XHCI)
+					continue;
+
+				if (xdbc_num-- != 0)

drivers/usb/early/xhci-dbc.c new file +21 -0

+/**
+ * struct xdbc_regs - xHCI Debug Capability Register interface.
+ */
+struct xdbc_regs {
+	__le32	capability;
+	__le32	doorbell;
+	__le32	ersts;		/* Event Ring Segment Table Size*/
+	__le32	__reserved_0;	/* 0c~0f reserved bits */
+	__le64	erstba;		/* Event Ring Segment Table Base Address */
+	__le64	erdp;		/* Event Ring Dequeue Pointer */
+	__le32	control;
+#define DEBUG_MAX_BURST(p)	(((p) >> 16) & 0xff)
+#define CTRL_DCR		BIT(0)		/* DbC Run */
+#define CTRL_PED		BIT(1)		/* Port Enable/Disable */
+#define CTRL_HOT		BIT(2)		/* Halt Out TR */
+#define CTRL_HIT		BIT(3)		/* Halt In TR */
+#define CTRL_DRC		BIT(4)		/* DbC run change */
+#define CTRL_DCE		BIT(31)		/* DbC enable */
+	__le32	status;
+#define DCST_DPN(p)		(((p) >> 24) & 0xff)
+	__le32	portsc;		/* Port status and control */

Quoted 6 lines · Lu Baolu

xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. Software learns this capability by walking through the extended capability list of the host. xHCI specification describes DbC in section 7.6.

Quoted 10 lines

This patch introduces the code to probe and initialize the debug capability hardware during early boot. With hardware initialized, the debug target (system on which this code is running) will present a debug device through the debug port (normally the first USB3 port). The debug device is fully compliant with the USB framework and provides the equivalent of a very high performance (USB3) full-duplex serial link between the debug host and target. The DbC functionality is independent of xHCI host. There isn't any precondition from xHCI host side for DbC to work.

s/xHCI host/the xHCI host

Quoted 3 lines

This patch also includes bulk out and bulk in interfaces. These interfaces could be used to implement early printk bootconsole or hook to various system debuggers.

s/out/output
s/in/input

arch/x86/Kconfig.debug config EARLY_PRINTK_EFI · line 56

Quoted 2 lines

arch/x86/Kconfig.debug +2 -0

+config EARLY_PRINTK_USB_XDBC
+	bool "Early printk via xHCI debug port"

s/xHCI/the xHCI

I remarked on this in my first review as well - mind checking the whole series for uses of 'xHCI'?

Quoted 106 lines

arch/x86/Kconfig.debug +11 -0

+	depends on EARLY_PRINTK && PCI
+	select EARLY_PRINTK_USB
+	---help---
+	  Write kernel log output directly into the xHCI debug port.
+
+	  This is useful for kernel debugging when your machine crashes very
+	  early before the console code is initialized. For normal operation
+	  it is not recommended because it looks ugly and doesn't cooperate
+	  with klogd/syslogd or the X server. You should normally N here,
+	  unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
 	def_bool n

drivers/usb/Kconfig +3 -0

index fbe493d..9313fff 100644
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
 config USB_EHCI_BIG_ENDIAN_DESC
 	bool
 
+config USB_EARLY_PRINTK
+	bool
+
 menuconfig USB_SUPPORT
 	bool "USB support"
 	depends on HAS_IOMEM

drivers/usb/Makefile +1 -1

index 7791af6..53d1356 100644
@@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)	+= image/
 obj-$(CONFIG_USB_SERIAL)	+= serial/
 
 obj-$(CONFIG_USB)		+= misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
+obj-$(CONFIG_EARLY_PRINTK_USB)	+= early/
 
 obj-$(CONFIG_USB_ATM)		+= atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)	+= atm/

drivers/usb/early/Makefile +1 -0

index 24bbe51..fcde228 100644
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_USB_XDBC) += xhci-dbc.o

drivers/usb/early/xhci-dbc.c new file +50 -0

new file mode 100644
index 0000000..72480c4
@@ -0,0 +1,1027 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ *
+ * 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)	KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include <linux/console.h>
+#include <linux/pci_regs.h>
+#include <linux/pci_ids.h>
+#include <linux/bootmem.h>
+#include <linux/io.h>
+#include <asm/pci-direct.h>
+#include <asm/fixmap.h>
+#include <linux/bcd.h>
+#include <linux/export.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int early_console_keep;
+
+#ifdef XDBC_TRACE
+#define	xdbc_trace	trace_printk
+#else
+static inline void xdbc_trace(const char *fmt, ...) { }
+#endif /* XDBC_TRACE */
+
+static int xdbc_bulk_transfer(void *data, int size, bool read);
+
+static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
+{
+	u32 val, sz;
+	u64 val64, sz64, mask64;
+	u8 byte;
+	void __iomem *base;
+
+	val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
+	write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);

drivers/usb/early/xhci-dbc.c line 60

Quoted 10 lines

drivers/usb/early/xhci-dbc.c new file +10 -0

+	sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
+	write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
+	if (val == 0xffffffff || sz == 0xffffffff) {
+		pr_notice("invalid mmio bar\n");
+		return NULL;
+	}
+
+	val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
+	sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
+	mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;

Is this cast necessary?

drivers/usb/early/xhci-dbc.c line 69

Quoted 9 lines

drivers/usb/early/xhci-dbc.c new file +9 -0

+
+	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
+		val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
+		write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
+		sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
+		write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
+
+		val64 |= (u64)val << 32;
+		sz64 |= (u64)sz << 32;

Could these type casts be avoided by declaring 'val' and 'sz' as u64 to begin with?

drivers/usb/early/xhci-dbc.c line 70

Quoted 1 line

drivers/usb/early/xhci-dbc.c new file +1 -0

+		mask64 |= (u64)~0 << 32;

Type cast could be avoided by using 0L.

Quoted 12 lines

drivers/usb/early/xhci-dbc.c new file +12 -0

+static u32 __init xdbc_find_dbgp(int xdbc_num, u32 *b, u32 *d, u32 *f)
+{
+	u32 bus, dev, func, class;
+
+	for (bus = 0; bus < XDBC_PCI_MAX_BUSES; bus++) {
+		for (dev = 0; dev < XDBC_PCI_MAX_DEVICES; dev++) {
+			for (func = 0; func < XDBC_PCI_MAX_FUNCTION; func++) {
+				class = read_pci_config(bus, dev, func, PCI_CLASS_REVISION);
+				if ((class >> 8) != PCI_CLASS_SERIAL_USB_XHCI)
+					continue;
+
+				if (xdbc_num-- != 0)

drivers/usb/early/xhci-dbc.c line 131

Quoted 10 lines

drivers/usb/early/xhci-dbc.c new file +10 -0

+					continue;
+
+				*b = bus;
+				*d = dev;
+				*f = func;
+
+				return 0;
+			}
+		}
+	}

Nit: to me it would look more balanced visually if the body of the innermost loop started with an empty newline.

drivers/usb/early/xhci-dbc.c line 175

Quoted 2 lines

drivers/usb/early/xhci-dbc.c new file +2 -0

+	ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
+						0, XHCI_EXT_CAPS_LEGACY);

Please don't break this line. You can shorten the variable name if you want to save line length.

drivers/usb/early/xhci-dbc.c line 341

Quoted 7 lines

drivers/usb/early/xhci-dbc.c new file +7 -0

+	/* populate the contexts */
+	context = (struct xdbc_context *)xdbc.dbcc_base;
+	context->info.string0 = cpu_to_le64(xdbc.string_dma);
+	context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
+	context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
+	context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
+	context->info.length = cpu_to_le32(string_length);

Wouldn't this (and some of the other initializations) look nicer this way:

	/* Populate the contexts: */
	context = (struct xdbc_context *)xdbc.dbcc_base;

	context->info.string0		= cpu_to_le64(xdbc.string_dma);
	context->info.manufacturer	= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
	context->info.product		= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
	context->info.serial		= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
	context->info.length		= cpu_to_le32(string_length);

?

drivers/usb/early/xhci-dbc.c line 901

Quoted 1 line

drivers/usb/early/xhci-dbc.c new file +1 -0

+	/* Check completion of the previous request. */

Could you please standardize the capitalization and punctuation of all the comments in the patches? Some are lower case, some are upper case, some have full stops, some don't.

One good style would be to use this form when a comment refers to what the next statement does:

	/* Check completion of the previous request: */

Quoted 21 lines

drivers/usb/early/xhci-dbc.c new file +21 -0

+/**
+ * struct xdbc_regs - xHCI Debug Capability Register interface.
+ */
+struct xdbc_regs {
+	__le32	capability;
+	__le32	doorbell;
+	__le32	ersts;		/* Event Ring Segment Table Size*/
+	__le32	__reserved_0;	/* 0c~0f reserved bits */
+	__le64	erstba;		/* Event Ring Segment Table Base Address */
+	__le64	erdp;		/* Event Ring Dequeue Pointer */
+	__le32	control;
+#define DEBUG_MAX_BURST(p)	(((p) >> 16) & 0xff)
+#define CTRL_DCR		BIT(0)		/* DbC Run */
+#define CTRL_PED		BIT(1)		/* Port Enable/Disable */
+#define CTRL_HOT		BIT(2)		/* Halt Out TR */
+#define CTRL_HIT		BIT(3)		/* Halt In TR */
+#define CTRL_DRC		BIT(4)		/* DbC run change */
+#define CTRL_DCE		BIT(31)		/* DbC enable */
+	__le32	status;
+#define DCST_DPN(p)		(((p) >> 24) & 0xff)
+	__le32	portsc;		/* Port status and control */

drivers/usb/early/xhci-dbc.c line 133

Quoted 10 lines

drivers/usb/early/xhci-dbc.c new file +10 -0

+#define PORTSC_CCS		BIT(0)
+#define PORTSC_CSC		BIT(17)
+#define PORTSC_PRC		BIT(21)
+#define PORTSC_PLC		BIT(22)
+#define PORTSC_CEC		BIT(23)
+	__le32	__reserved_1;	/* 2b~28 reserved bits */
+	__le64	dccp;		/* Debug Capability Context Pointer */
+	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
+	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
+};

So why are defines intermixed with the fields? If the defines relate to the field then their naming sure does not express that ...

I was thinking of something like:

/**
* struct xdbc_regs - xHCI Debug Capability Register interface. */

struct xdbc_regs {
	__le32	capability;
	__le32	doorbell;
	__le32	ersts;		/* Event Ring Segment Table Size*/
	__le32	__reserved_0;	/* 0c~0f reserved bits */
	__le64	erstba;		/* Event Ring Segment Table Base Address */
	__le64	erdp;		/* Event Ring Dequeue Pointer */
	__le32	control;
	__le32	status;
	__le32	portsc;		/* Port status and control */
	__le32	__reserved_1;	/* 2b~28 reserved bits */
	__le64	dccp;		/* Debug Capability Context Pointer */
	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
};

#define DEBUG_MAX_BURST(p) (((p) >> 16) & 0xff)

#define CTRL_DCR BIT(0) /* DbC Run */
#define CTRL_PED BIT(1) /* Port Enable/Disable */
#define CTRL_HOT BIT(2) /* Halt Out TR */
#define CTRL_HIT BIT(3) /* Halt In TR */
#define CTRL_DRC BIT(4) /* DbC run change */
#define CTRL_DCE BIT(31) /* DbC enable */

#define DCST_DPN(p) (((p) >> 24) & 0xff)

#define PORTSC_CCS BIT(0)
#define PORTSC_CSC BIT(17)
#define PORTSC_PRC BIT(21)
#define PORTSC_PLC BIT(22)
#define PORTSC_CEC BIT(23)

Also, the abbreviations suck big time. What's wrong with:

#define CTRL_DBC_RUN BIT(0)
#define CTRL_PORT_ENABLE BIT(1)
#define CTRL_HALT_OUT_TR BIT(2)
#define CTRL_HALT_IN_TR BIT(3)
#define CTRL_DBC_RUN_CHANGE BIT(4)
#define CTRL_DBC_ENABLE BIT(31)

?

Also note how the comments became superfluous once descriptive names are chosen...

Please review the rest of the defines and series for similar problems and fix them, there are more of the same type.