[PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Lu Baolu]

xHCI debug capability (DbC) is an optional but standalone
functionality provided by an xHCI host controller. With DbC
hardware initialized, the system will present a debug device
through the USB3 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 set adds support for early printk functionality
through a USB3 debug port by 1) initializing and enabling
the DbC hardware during early boot; 2) registering a boot
console to the system so that early printk messages can go
through the USB3 debug port. It also includes some lines
of changes in usb_debug driver so that it can be bound when
a USB3 debug device is enumerated.

This code is designed to be used only for kernel debugging
when machine crashes very early before the console code is
initialized. It makes the life of kernel debugging easier
when people work with a modern machine without any legacy
serial ports.

---
Change log:
v4->v5:
  - add raw_spin_lock to make xdbc_bulk_write() reentrant. 

v3->v4:
  - Rename the document with .dst suffix.
  - Add the list of hardware that has been succesfuly
    tested on in the document.

v2->v3:
  - Removed spinlock usage.
  - Removed work queue usage.
  - Refined the user guide document.

v1->v2:
  - Refactor the duplicate code in xdbc_early_start() and
    xdbc_handle_external_reset().
  - Free resources when hardware not used any more.
  - Refine the user guide document.

Lu Baolu (4):
  usb: dbc: early driver for xhci debug capability
  x86: add support for earlyprintk via USB3 debug port
  usb: serial: usb_debug: add support for dbc debug device
  usb: doc: add document for USB3 debug port usage

 Documentation/kernel-parameters.txt   |    1 +
 Documentation/usb/usb3-debug-port.rst |   95 +++
 arch/x86/Kconfig.debug                |   14 +
 arch/x86/kernel/early_printk.c        |    5 +
 arch/x86/kernel/setup.c               |    7 +
 drivers/usb/Kconfig                   |    3 +
 drivers/usb/Makefile                  |    2 +-
 drivers/usb/early/Makefile            |    1 +
 drivers/usb/early/xhci-dbc.c          | 1068 +++++++++++++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h          |  205 +++++++
 drivers/usb/serial/usb_debug.c        |   28 +-
 include/linux/usb/xhci-dbgp.h         |   22 +
 12 files changed, 1447 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/usb/usb3-debug-port.rst
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

-- 
2.1.4

[PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[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.

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.

This code is designed to be only used for kernel debugging
when machine crashes very early before the console code is
initialized. For normal operation it is not recommended.

Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/x86/Kconfig.debug        |   14 +
 drivers/usb/Kconfig           |    3 +
 drivers/usb/Makefile          |    2 +-
 drivers/usb/early/Makefile    |    1 +
 drivers/usb/early/xhci-dbc.c  | 1068 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h  |  205 ++++++++
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1314 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..13e85b7 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
 	bool "Early printk via EHCI debug port"
 	depends on EARLY_PRINTK && PCI
+	select USB_EARLY_PRINTK
 	---help---
 	  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
 	  This is useful for kernel debugging when your machine crashes very
 	  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+	bool "Early printk via xHCI debug port"
+	depends on EARLY_PRINTK && PCI
+	select USB_EARLY_PRINTK
+	---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
 
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index fbe493d..9313fff 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -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
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 7791af6..0c37838 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -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_USB_EARLY_PRINTK)	+= early/
 
 obj-$(CONFIG_USB_ATM)		+= atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)	+= atm/
diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
index 24bbe51..2db5906 100644
--- a/drivers/usb/early/Makefile
+++ b/drivers/usb/early/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
new file mode 100644
index 0000000..5ac4223
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1068 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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);
+	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;
+
+	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);
+		mask64 |= ((u64)~0 << 32);
+	}
+
+	sz64 &= mask64;
+
+	if (sizeof(dma_addr_t) < 8 || !sz64) {
+		pr_notice("invalid mmio address\n");
+		return NULL;
+	}
+
+	sz64 = 1ULL << __ffs64(sz64);
+
+	/* check if the mem space is enabled */
+	byte = read_pci_config_byte(bus, dev, func, PCI_COMMAND);
+	if (!(byte & PCI_COMMAND_MEMORY)) {
+		byte  |= PCI_COMMAND_MEMORY;
+		write_pci_config_byte(bus, dev, func, PCI_COMMAND, byte);
+	}
+
+	xdbc.xhci_start = val64;
+	xdbc.xhci_length = sz64;
+	base = early_ioremap(val64, sz64);
+
+	return base;
+}
+
+static void * __init xdbc_get_page(dma_addr_t *dma_addr)
+{
+	void *virt;
+
+	virt = alloc_bootmem_pages_nopanic(PAGE_SIZE);
+	if (!virt)
+		return NULL;
+
+	if (dma_addr)
+		*dma_addr = (dma_addr_t)__pa(virt);
+
+	return virt;
+}
+
+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)
+					continue;
+
+				*b = bus;
+				*d = dev;
+				*f = func;
+
+				return 0;
+			}
+		}
+	}
+
+	return -1;
+}
+
+static void xdbc_early_delay(unsigned long count)
+{
+	u8 val;
+
+	val = inb(0x80);
+	while (count-- > 0)
+		outb(val, 0x80);
+}
+
+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
+
+static int handshake(void __iomem *ptr, u32 mask, u32 done,
+		     int wait, int delay)
+{
+	u32 result;
+
+	do {
+		result = readl(ptr);
+		result &= mask;
+		if (result == done)
+			return 0;
+		xdbc_delay(delay);
+		wait -= delay;
+	} while (wait > 0);
+
+	return -ETIMEDOUT;
+}
+
+static void __init xdbc_bios_handoff(void)
+{
+	int ext_cap_offset;
+	int timeout;
+	u32 val;
+
+	ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
+						0, XHCI_EXT_CAPS_LEGACY);
+	val = readl(xdbc.xhci_base + ext_cap_offset);
+
+	/* If the BIOS owns the HC, signal that the OS wants it, and wait */
+	if (val & XHCI_HC_BIOS_OWNED) {
+		writel(val | XHCI_HC_OS_OWNED,
+		       xdbc.xhci_base + ext_cap_offset);
+		timeout = handshake(xdbc.xhci_base + ext_cap_offset,
+				    XHCI_HC_BIOS_OWNED, 0, 5000, 10);
+
+		/* Assume a buggy BIOS and take HC ownership anyway */
+		if (timeout) {
+			pr_notice("xHCI BIOS handoff failed\n");
+			writel(val & ~XHCI_HC_BIOS_OWNED,
+			       xdbc.xhci_base + ext_cap_offset);
+		}
+	}
+
+	/* Disable any BIOS SMIs and clear all SMI events*/
+	val = readl(xdbc.xhci_base + ext_cap_offset +
+		    XHCI_LEGACY_CONTROL_OFFSET);
+	val &= XHCI_LEGACY_DISABLE_SMI;
+	val |= XHCI_LEGACY_SMI_EVENTS;
+	writel(val, xdbc.xhci_base + ext_cap_offset +
+	       XHCI_LEGACY_CONTROL_OFFSET);
+}
+
+static int __init xdbc_alloc_ring(struct xdbc_segment *seg,
+				  struct xdbc_ring *ring)
+{
+	seg->trbs = xdbc_get_page(&seg->dma);
+	if (!seg->trbs)
+		return -ENOMEM;
+
+	ring->segment = seg;
+
+	return 0;
+}
+
+static void __init xdbc_free_ring(struct xdbc_ring *ring)
+{
+	struct xdbc_segment *seg = ring->segment;
+
+	if (!seg)
+		return;
+
+	free_bootmem(seg->dma, PAGE_SIZE);
+	ring->segment = NULL;
+}
+
+static void xdbc_reset_ring(struct xdbc_ring *ring)
+{
+	struct xdbc_trb *link_trb;
+	struct xdbc_segment *seg = ring->segment;
+
+	memset(seg->trbs, 0, PAGE_SIZE);
+
+	ring->enqueue = seg->trbs;
+	ring->dequeue = seg->trbs;
+	ring->cycle_state = 1;
+
+	if (ring != &xdbc.evt_ring) {
+		link_trb = &seg->trbs[XDBC_TRBS_PER_SEGMENT - 1];
+		link_trb->field[0] = cpu_to_le32(lower_32_bits(seg->dma));
+		link_trb->field[1] = cpu_to_le32(upper_32_bits(seg->dma));
+		link_trb->field[3] = cpu_to_le32(TRB_TYPE(TRB_LINK)) |
+						 cpu_to_le32(LINK_TOGGLE);
+	}
+}
+
+static inline void xdbc_put_utf16(u16 *s, const char *c, size_t size)
+{
+	int i;
+
+	for (i = 0; i < size; i++)
+		s[i] = cpu_to_le16(c[i]);
+}
+
+static void xdbc_mem_init(void)
+{
+	struct xdbc_erst_entry *entry;
+	struct xdbc_strings *strings;
+	struct xdbc_context *context;
+	struct xdbc_ep_context *ep_in, *ep_out;
+	struct usb_string_descriptor *s_desc;
+	unsigned int max_burst;
+	u32 string_length;
+	int index = 0;
+	u32 dev_info;
+
+	xdbc_reset_ring(&xdbc.evt_ring);
+	xdbc_reset_ring(&xdbc.in_ring);
+	xdbc_reset_ring(&xdbc.out_ring);
+	memset(xdbc.table_base, 0, PAGE_SIZE);
+	memset(xdbc.out_buf, 0, PAGE_SIZE);
+
+	/* Initialize event ring segment table */
+	xdbc.erst_size = 16;
+	xdbc.erst_base = xdbc.table_base +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	xdbc.erst_dma = xdbc.table_dma +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	index += XDBC_ERST_ENTRY_NUM;
+
+	/* Initialize Event Ring Segment Table */
+	entry = (struct xdbc_erst_entry *)xdbc.erst_base;
+	entry->seg_addr = cpu_to_le64(xdbc.evt_seg.dma);
+	entry->seg_size = cpu_to_le32(XDBC_TRBS_PER_SEGMENT);
+	entry->rsvd = 0;
+
+	/* Initialize ERST registers */
+	writel(1, &xdbc.xdbc_reg->ersts);
+	xdbc_write64(xdbc.erst_dma, &xdbc.xdbc_reg->erstba);
+	xdbc_write64(xdbc.evt_seg.dma, &xdbc.xdbc_reg->erdp);
+
+	/* debug capability contexts */
+	BUILD_BUG_ON(sizeof(struct xdbc_info_context) != 64);
+	BUILD_BUG_ON(sizeof(struct xdbc_ep_context) != 64);
+	BUILD_BUG_ON(sizeof(struct xdbc_context) != 64 * 3);
+
+	xdbc.dbcc_size = 64 * 3;
+	xdbc.dbcc_base = xdbc.table_base +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	xdbc.dbcc_dma = xdbc.table_dma +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	index += XDBC_DBCC_ENTRY_NUM;
+
+	/* strings */
+	xdbc.string_size = sizeof(struct xdbc_strings);
+	xdbc.string_base = xdbc.table_base +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	xdbc.string_dma = xdbc.table_dma +
+			index * XDBC_TABLE_ENTRY_SIZE;
+	index += XDBC_STRING_ENTRY_NUM;
+
+	strings = (struct xdbc_strings *)xdbc.string_base;
+
+	/* serial string */
+	s_desc = (struct usb_string_descriptor *)strings->serial;
+	s_desc->bLength = (strlen(XDBC_STRING_SERIAL) + 1) * 2;
+	s_desc->bDescriptorType = USB_DT_STRING;
+	xdbc_put_utf16(s_desc->wData, XDBC_STRING_SERIAL,
+		       strlen(XDBC_STRING_SERIAL));
+
+	string_length = s_desc->bLength;
+	string_length <<= 8;
+
+	/* product string */
+	s_desc = (struct usb_string_descriptor *)strings->product;
+	s_desc->bLength = (strlen(XDBC_STRING_PRODUCT) + 1) * 2;
+	s_desc->bDescriptorType = USB_DT_STRING;
+	xdbc_put_utf16(s_desc->wData, XDBC_STRING_PRODUCT,
+		       strlen(XDBC_STRING_PRODUCT));
+
+	string_length += s_desc->bLength;
+	string_length <<= 8;
+
+	/* manufacture string */
+	s_desc = (struct usb_string_descriptor *)strings->manufacture;
+	s_desc->bLength = (strlen(XDBC_STRING_MANUFACTURE) + 1) * 2;
+	s_desc->bDescriptorType = USB_DT_STRING;
+	xdbc_put_utf16(s_desc->wData, XDBC_STRING_MANUFACTURE,
+		       strlen(XDBC_STRING_MANUFACTURE));
+
+	string_length += s_desc->bLength;
+	string_length <<= 8;
+
+	/* string 0 */
+	strings->string0[0] = 4;
+	strings->string0[1] = USB_DT_STRING;
+	strings->string0[2] = 0x09;
+	strings->string0[3] = 0x04;
+
+	string_length += 4;
+
+	/* populate the contexts */
+	context = (struct xdbc_context *)xdbc.dbcc_base;
+	context->info.string0 = cpu_to_le64(xdbc.string_dma);
+	context->info.manufacture = 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);
+
+	max_burst = DEBUG_MAX_BURST(readl(&xdbc.xdbc_reg->control));
+	ep_out = (struct xdbc_ep_context *)&context->out;
+	ep_out->ep_info1 = 0;
+	ep_out->ep_info2 = cpu_to_le32(EP_TYPE(BULK_OUT_EP) |
+			MAX_PACKET(1024) | MAX_BURST(max_burst));
+	ep_out->deq = cpu_to_le64(xdbc.out_seg.dma |
+			xdbc.out_ring.cycle_state);
+
+	ep_in = (struct xdbc_ep_context *)&context->in;
+	ep_in->ep_info1 = 0;
+	ep_in->ep_info2 = cpu_to_le32(EP_TYPE(BULK_OUT_EP) |
+			MAX_PACKET(1024) | MAX_BURST(max_burst));
+	ep_in->deq = cpu_to_le64(xdbc.in_seg.dma |
+			xdbc.in_ring.cycle_state);
+
+	/* write DbC context pointer register */
+	xdbc_write64(xdbc.dbcc_dma, &xdbc.xdbc_reg->dccp);
+
+	/* device descriptor info registers */
+	dev_info = cpu_to_le32((XDBC_VENDOR_ID << 16) | XDBC_PROTOCOL);
+	writel(dev_info, &xdbc.xdbc_reg->devinfo1);
+	dev_info = cpu_to_le32((XDBC_DEVICE_REV << 16) | XDBC_PRODUCT_ID);
+	writel(dev_info, &xdbc.xdbc_reg->devinfo2);
+
+	xdbc.in_buf = xdbc.out_buf + XDBC_MAX_PACKET;
+	xdbc.in_dma = xdbc.out_dma + XDBC_MAX_PACKET;
+}
+
+static void xdbc_do_reset_debug_port(u32 id, u32 count)
+{
+	u32 val, cap_length;
+	void __iomem *ops_reg;
+	void __iomem *portsc;
+	int i;
+
+	cap_length = readl(xdbc.xhci_base) & 0xff;
+	ops_reg = xdbc.xhci_base + cap_length;
+
+	id--;
+	for (i = id; i < (id + count); i++) {
+		portsc = ops_reg + 0x400 + i * 0x10;
+		val = readl(portsc);
+		/* reset the port if CCS bit is cleared */
+		if (!(val & PORT_CONNECT))
+			writel(val | PORT_RESET, portsc);
+	}
+}
+
+static void __init xdbc_reset_debug_port(void)
+{
+	int offset = 0;
+	u32 val, port_offset, port_count;
+
+	do {
+		offset = xhci_find_next_ext_cap(xdbc.xhci_base,
+						offset,
+						XHCI_EXT_CAPS_PROTOCOL);
+		if (!offset)
+			break;
+
+		val = readl(xdbc.xhci_base + offset);
+		if (XHCI_EXT_PORT_MAJOR(val) != 0x3)
+			continue;
+
+		val = readl(xdbc.xhci_base + offset + 8);
+		port_offset = XHCI_EXT_PORT_OFF(val);
+		port_count = XHCI_EXT_PORT_COUNT(val);
+
+		xdbc_do_reset_debug_port(port_offset, port_count);
+	} while (1);
+}
+
+static void xdbc_queue_trb(struct xdbc_ring *ring,
+			   u32 field1, u32 field2, u32 field3, u32 field4)
+{
+	struct xdbc_trb *trb, *link_trb;
+
+	trb = ring->enqueue;
+	trb->field[0] = cpu_to_le32(field1);
+	trb->field[1] = cpu_to_le32(field2);
+	trb->field[2] = cpu_to_le32(field3);
+	trb->field[3] = cpu_to_le32(field4);
+
+	++(ring->enqueue);
+	if (ring->enqueue >= &ring->segment->trbs[TRBS_PER_SEGMENT - 1]) {
+		link_trb = ring->enqueue;
+		if (ring->cycle_state)
+			link_trb->field[3] |= cpu_to_le32(TRB_CYCLE);
+		else
+			link_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
+
+		ring->enqueue = ring->segment->trbs;
+		ring->cycle_state ^= 1;
+	}
+}
+
+static void xdbc_ring_doorbell(int target)
+{
+	writel(DOOR_BELL_TARGET(target), &xdbc.xdbc_reg->doorbell);
+}
+
+static int xdbc_start(void)
+{
+	u32 ctrl, status;
+	int ret;
+
+	ctrl = readl(&xdbc.xdbc_reg->control);
+	writel(ctrl | CTRL_DCE | CTRL_PED, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control,
+			CTRL_DCE, CTRL_DCE, 100000, 100);
+	if (ret) {
+		xdbc_trace("failed to initialize hardware\n");
+		return ret;
+	}
+
+	/* reset port to avoid bus hang */
+	if (xdbc.vendor == PCI_VENDOR_ID_INTEL)
+		xdbc_reset_debug_port();
+
+	/* wait for port connection */
+	ret = handshake(&xdbc.xdbc_reg->portsc,
+			PORTSC_CCS, PORTSC_CCS, 5000000, 100);
+	if (ret) {
+		xdbc_trace("waiting for connection timed out\n");
+		return ret;
+	}
+
+	/* wait for debug device to be configured */
+	ret = handshake(&xdbc.xdbc_reg->control,
+			CTRL_DCR, CTRL_DCR, 5000000, 100);
+	if (ret) {
+		xdbc_trace("waiting for device configuration timed out\n");
+		return ret;
+	}
+
+	/* port should have a valid port# */
+	status = readl(&xdbc.xdbc_reg->status);
+	if (!DCST_DPN(status)) {
+		xdbc_trace("invalid root hub port number\n");
+		return -ENODEV;
+	}
+
+	xdbc.port_number = DCST_DPN(status);
+
+	xdbc_trace("DbC is running now, control 0x%08x port ID %d\n",
+		   readl(&xdbc.xdbc_reg->control), xdbc.port_number);
+
+	return 0;
+}
+
+static int xdbc_handle_external_reset(void)
+{
+	int ret = 0;
+	static int failure_count;
+
+	xdbc_trace("external reset detected\n");
+
+	if (failure_count >= 5)
+		return -ENODEV;
+
+	xdbc.flags = 0;
+	writel(0, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control, CTRL_DCE, 0, 100000, 10);
+	if (ret)
+		goto count_and_out;
+
+	xdbc_mem_init();
+
+	mmiowb();
+
+	ret = xdbc_start();
+	if (ret < 0)
+		goto count_and_out;
+
+	xdbc_trace("dbc recovered\n");
+
+	xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
+
+	xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+
+	return 0;
+
+count_and_out:
+	failure_count++;
+	xdbc_trace("failed to recover from external reset, retried %d\n",
+		   failure_count);
+	return ret;
+}
+
+static int xdbc_bulk_transfer(void *data, int size, bool read)
+{
+	u64 addr;
+	u32 length, control;
+	struct xdbc_trb *trb;
+	struct xdbc_ring *ring;
+	u32 cycle;
+	int ret;
+
+	if (size > XDBC_MAX_PACKET) {
+		xdbc_trace("oops: bad parameter, size %d\n", size);
+		return -EINVAL;
+	}
+
+	if (!(readl(&xdbc.xdbc_reg->control) & CTRL_DCE)) {
+		ret = xdbc_handle_external_reset();
+		if (ret) {
+			xdbc_trace("oops: hardware failed to recover\n");
+			return -EIO;
+		}
+	}
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED) ||
+	    !(xdbc.flags & XDBC_FLAGS_CONFIGURED) ||
+	    (!read && (xdbc.flags & XDBC_FLAGS_BULKOUT_STALL)) ||
+	    (read && (xdbc.flags & XDBC_FLAGS_BULKIN_STALL))) {
+		xdbc_trace("oops: hardware not ready %08x\n", xdbc.flags);
+		return -EIO;
+	}
+
+	ring = (read ? &xdbc.in_ring : &xdbc.out_ring);
+	trb = ring->enqueue;
+	cycle = ring->cycle_state;
+
+	length = TRB_LEN(size);
+	control = TRB_TYPE(TRB_NORMAL) | TRB_IOC;
+
+	if (cycle)
+		control &= cpu_to_le32(~TRB_CYCLE);
+	else
+		control |= cpu_to_le32(TRB_CYCLE);
+
+	if (read) {
+		memset(xdbc.in_buf, 0, XDBC_MAX_PACKET);
+		addr = xdbc.in_dma;
+		xdbc.flags |= XDBC_FLAGS_IN_PROCESS;
+	} else {
+		memset(xdbc.out_buf, 0, XDBC_MAX_PACKET);
+		memcpy(xdbc.out_buf, data, size);
+		addr = xdbc.out_dma;
+		xdbc.flags |= XDBC_FLAGS_OUT_PROCESS;
+	}
+
+	xdbc_queue_trb(ring, lower_32_bits(addr),
+		       upper_32_bits(addr),
+		       length, control);
+
+	/*
+	 * Memory barrier to ensure hardware sees the trbs
+	 * enqueued above.
+	 */
+	wmb();
+	if (cycle)
+		trb->field[3] |= cpu_to_le32(cycle);
+	else
+		trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
+
+	xdbc_ring_doorbell(read ? IN_EP_DOORBELL : OUT_EP_DOORBELL);
+
+	return size;
+}
+
+static int __init xdbc_early_setup(void)
+{
+	int ret;
+
+	writel(0, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control, CTRL_DCE, 0, 100000, 100);
+	if (ret)
+		return ret;
+
+	/* allocate table page */
+	xdbc.table_base = xdbc_get_page(&xdbc.table_dma);
+	if (!xdbc.table_base)
+		return -ENOMEM;
+
+	/* get and store the transfer buffer */
+	xdbc.out_buf = xdbc_get_page(&xdbc.out_dma);
+	if (!xdbc.out_buf)
+		return -ENOMEM;
+
+	/* allocate event ring */
+	ret = xdbc_alloc_ring(&xdbc.evt_seg, &xdbc.evt_ring);
+	if (ret < 0)
+		return ret;
+
+	/* IN/OUT endpoint transfer ring */
+	ret = xdbc_alloc_ring(&xdbc.in_seg, &xdbc.in_ring);
+	if (ret < 0)
+		return ret;
+
+	ret = xdbc_alloc_ring(&xdbc.out_seg, &xdbc.out_ring);
+	if (ret < 0)
+		return ret;
+
+	xdbc_mem_init();
+
+	/*
+	 * Memory barrier to ensure hardware sees the bits
+	 * setting above.
+	 */
+	mmiowb();
+
+	ret = xdbc_start();
+	if (ret < 0) {
+		/* give the shared port back to host */
+		writel(0, &xdbc.xdbc_reg->control);
+		return ret;
+	}
+
+	xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
+
+	xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+
+	return 0;
+}
+
+int __init early_xdbc_parse_parameter(char *s)
+{
+	unsigned long dbgp_num = 0;
+	u32 bus, dev, func, offset;
+	int ret;
+
+	if (!early_pci_allowed())
+		return -EPERM;
+
+	if (strstr(s, "keep"))
+		early_console_keep = 1;
+
+	if (xdbc.xdbc_reg)
+		return 0;
+
+	if (*s && kstrtoul(s, 0, &dbgp_num))
+		dbgp_num = 0;
+
+	pr_notice("dbgp_num: %lu\n", dbgp_num);
+
+	/* Locate the host controller */
+	ret = xdbc_find_dbgp(dbgp_num, &bus, &dev, &func);
+	if (ret) {
+		pr_notice("no host controller found in your system\n");
+		return -ENODEV;
+	}
+	xdbc.vendor = read_pci_config_16(bus, dev, func, PCI_VENDOR_ID);
+	xdbc.device = read_pci_config_16(bus, dev, func, PCI_DEVICE_ID);
+	xdbc.bus = bus;
+	xdbc.dev = dev;
+	xdbc.func = func;
+
+	/* Map the IO memory */
+	xdbc.xhci_base = xdbc_map_pci_mmio(bus, dev, func);
+	if (!xdbc.xhci_base)
+		return -EINVAL;
+
+	/* Locate DbC registers */
+	offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
+	if (!offset) {
+		pr_notice("DbC wasn't found in your host controller\n");
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+		xdbc.xhci_base = NULL;
+		xdbc.xhci_length = 0;
+
+		return -ENODEV;
+	}
+	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
+
+	return 0;
+}
+
+int __init early_xdbc_setup_hardware(void)
+{
+	int ret;
+
+	if (!xdbc.xdbc_reg)
+		return -ENODEV;
+
+	/* hand over the owner of host from BIOS */
+	xdbc_bios_handoff();
+
+	raw_spin_lock_init(&xdbc.lock);
+
+	ret = xdbc_early_setup();
+	if (ret) {
+		pr_notice("failed to setup DbC hardware\n");
+
+		xdbc_free_ring(&xdbc.evt_ring);
+		xdbc_free_ring(&xdbc.out_ring);
+		xdbc_free_ring(&xdbc.in_ring);
+
+		if (xdbc.table_dma)
+			free_bootmem(xdbc.table_dma, PAGE_SIZE);
+
+		if (xdbc.out_dma)
+			free_bootmem(xdbc.out_dma, PAGE_SIZE);
+
+		xdbc.table_base = NULL;
+		xdbc.out_buf = NULL;
+	}
+
+	return ret;
+}
+
+static void xdbc_handle_port_status(struct xdbc_trb *evt_trb)
+{
+	u32 port_reg;
+
+	port_reg = readl(&xdbc.xdbc_reg->portsc);
+
+	if (port_reg & PORTSC_CSC) {
+		xdbc_trace("%s: connect status change event\n", __func__);
+		writel(port_reg | PORTSC_CSC, &xdbc.xdbc_reg->portsc);
+		port_reg = readl(&xdbc.xdbc_reg->portsc);
+	}
+
+	if (port_reg & PORTSC_PRC) {
+		xdbc_trace("%s: port reset change event\n", __func__);
+		writel(port_reg | PORTSC_PRC, &xdbc.xdbc_reg->portsc);
+		port_reg = readl(&xdbc.xdbc_reg->portsc);
+	}
+
+	if (port_reg & PORTSC_PLC) {
+		xdbc_trace("%s: port link status change event\n", __func__);
+		writel(port_reg | PORTSC_PLC, &xdbc.xdbc_reg->portsc);
+		port_reg = readl(&xdbc.xdbc_reg->portsc);
+	}
+
+	if (port_reg & PORTSC_CEC) {
+		xdbc_trace("%s: config error change\n", __func__);
+		writel(port_reg | PORTSC_CEC, &xdbc.xdbc_reg->portsc);
+		port_reg = readl(&xdbc.xdbc_reg->portsc);
+	}
+}
+
+static void xdbc_handle_tx_event(struct xdbc_trb *evt_trb)
+{
+	u32 comp_code;
+	size_t remain_length;
+	int ep_id;
+
+	comp_code = GET_COMP_CODE(le32_to_cpu(evt_trb->field[2]));
+	remain_length = EVENT_TRB_LEN(le32_to_cpu(evt_trb->field[2]));
+	ep_id = TRB_TO_EP_ID(le32_to_cpu(evt_trb->field[3]));
+
+	/*
+	 * Possible Completion Codes for DbC Transfer Event are Success,
+	 * Stall Error, USB Transaction Error, Babble Detected Error,
+	 * TRB Error, Short Packet, Undefined Error, Event Ring Full Error,
+	 * and Vendor Defined Error. TRB error, undefined error and vendor
+	 * defined error will result in HOT/HIT set and be handled the same
+	 * way as Stall error.
+	 */
+	switch (comp_code) {
+	case COMP_SUCCESS:
+		remain_length = 0;
+	case COMP_SHORT_TX:
+		xdbc_trace("%s: endpoint %d remains %ld bytes\n", __func__,
+			   ep_id, remain_length);
+		break;
+	case COMP_TRB_ERR:
+	case COMP_BABBLE:
+	case COMP_TX_ERR:
+	case COMP_STALL:
+	default:
+		if (ep_id == XDBC_EPID_OUT)
+			xdbc.flags |= XDBC_FLAGS_BULKOUT_STALL;
+		if (ep_id == XDBC_EPID_IN)
+			xdbc.flags |= XDBC_FLAGS_BULKIN_STALL;
+
+		xdbc_trace("%s: endpoint %d stalled\n", __func__, ep_id);
+		break;
+	}
+
+	if (ep_id == XDBC_EPID_IN) {
+		xdbc.flags &= ~XDBC_FLAGS_IN_PROCESS;
+		xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+	} else if (ep_id == XDBC_EPID_OUT) {
+		xdbc.flags &= ~XDBC_FLAGS_OUT_PROCESS;
+	} else {
+		xdbc_trace("%s: invalid endpoint id %d\n", __func__, ep_id);
+	}
+}
+
+static void xdbc_handle_events(void)
+{
+	struct xdbc_trb *evt_trb;
+	bool update_erdp = false;
+	u8 command;
+	u32 reg;
+
+	command = read_pci_config_byte(xdbc.bus, xdbc.dev,
+				       xdbc.func, PCI_COMMAND);
+	if (!(command & PCI_COMMAND_MASTER)) {
+		command |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY;
+		write_pci_config_byte(xdbc.bus, xdbc.dev,
+				      xdbc.func, PCI_COMMAND, command);
+	}
+
+	/* check and handle configure-exit event */
+	reg = readl(&xdbc.xdbc_reg->control);
+	if (reg & CTRL_DRC) {
+		xdbc.flags &= ~XDBC_FLAGS_CONFIGURED;
+		writel(reg | CTRL_DRC, &xdbc.xdbc_reg->control);
+	} else {
+		xdbc.flags |= XDBC_FLAGS_CONFIGURED;
+	}
+
+	/* check endpoint stall event */
+	reg = readl(&xdbc.xdbc_reg->control);
+	if (reg & CTRL_HIT) {
+		xdbc.flags |= XDBC_FLAGS_BULKIN_STALL;
+	} else {
+		xdbc.flags &= ~XDBC_FLAGS_BULKIN_STALL;
+		if (!(xdbc.flags & XDBC_FLAGS_IN_PROCESS))
+			xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+	}
+
+	if (reg & CTRL_HOT)
+		xdbc.flags |= XDBC_FLAGS_BULKOUT_STALL;
+	else
+		xdbc.flags &= ~XDBC_FLAGS_BULKOUT_STALL;
+
+	evt_trb = xdbc.evt_ring.dequeue;
+	while ((le32_to_cpu(evt_trb->field[3]) & TRB_CYCLE) ==
+			xdbc.evt_ring.cycle_state) {
+		/*
+		 * Memory barrier to ensure software sees the trbs
+		 * enqueued by hardware.
+		 */
+		rmb();
+
+		switch ((le32_to_cpu(evt_trb->field[3]) & TRB_TYPE_BITMASK)) {
+		case TRB_TYPE(TRB_PORT_STATUS):
+			xdbc_handle_port_status(evt_trb);
+			break;
+		case TRB_TYPE(TRB_TRANSFER):
+			xdbc_handle_tx_event(evt_trb);
+			break;
+		default:
+			break;
+		}
+
+		/* advance to the next trb */
+		++(xdbc.evt_ring.dequeue);
+		if (xdbc.evt_ring.dequeue ==
+				&xdbc.evt_seg.trbs[TRBS_PER_SEGMENT]) {
+			xdbc.evt_ring.dequeue = xdbc.evt_seg.trbs;
+			xdbc.evt_ring.cycle_state ^= 1;
+		}
+
+		evt_trb = xdbc.evt_ring.dequeue;
+		update_erdp = true;
+	}
+
+	/* update event ring dequeue pointer */
+	if (update_erdp)
+		xdbc_write64(__pa(xdbc.evt_ring.dequeue),
+			     &xdbc.xdbc_reg->erdp);
+}
+
+static int xdbc_bulk_write(const char *bytes, int size)
+{
+	unsigned long flags;
+	int ret, timeout = 0;
+
+retry:
+	if (in_nmi()) {
+		if (!raw_spin_trylock_irqsave(&xdbc.lock, flags))
+			return -EAGAIN;
+	} else {
+		raw_spin_lock_irqsave(&xdbc.lock, flags);
+	}
+
+	xdbc_handle_events();
+
+	/* Check completion of the previous request. */
+	if ((xdbc.flags & XDBC_FLAGS_OUT_PROCESS) &&
+	    (timeout < 1000000)) {
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+		xdbc_delay(100);
+		timeout += 100;
+		goto retry;
+	}
+
+	if (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+
+		/*
+		 * Oops, hardware wasn't able to complete the
+		 * previous transfer.
+		 */
+		xdbc_trace("oops: previous transfer not completed yet\n");
+
+		return -ETIMEDOUT;
+	}
+
+	ret = xdbc_bulk_transfer((void *)bytes, size, false);
+
+	raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+
+	return ret;
+}
+
+static void early_xdbc_write(struct console *con, const char *str, u32 n)
+{
+	int chunk, ret;
+	static char buf[XDBC_MAX_PACKET];
+	int use_cr = 0;
+
+	if (!xdbc.xdbc_reg)
+		return;
+	memset(buf, 0, XDBC_MAX_PACKET);
+	while (n > 0) {
+		for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
+				str++, chunk++, n--) {
+			if (!use_cr && *str == '\n') {
+				use_cr = 1;
+				buf[chunk] = '\r';
+				str--;
+				n++;
+				continue;
+			}
+			if (use_cr)
+				use_cr = 0;
+			buf[chunk] = *str;
+		}
+		if (chunk > 0) {
+			ret = xdbc_bulk_write(buf, chunk);
+			if (ret < 0)
+				break;
+		}
+	}
+}
+
+static struct console early_xdbc_console = {
+	.name =		"earlyxdbc",
+	.write =	early_xdbc_write,
+	.flags =	CON_PRINTBUFFER,
+	.index =	-1,
+};
+
+void __init early_xdbc_register_console(void)
+{
+	if (early_console)
+		return;
+
+	early_console = &early_xdbc_console;
+	if (early_console_keep)
+		early_console->flags &= ~CON_BOOT;
+	else
+		early_console->flags |= CON_BOOT;
+	register_console(early_console);
+}
+
+static int __init xdbc_init(void)
+{
+	unsigned long flags;
+	void __iomem *base;
+	u32 offset;
+	int ret = 0;
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
+		return 0;
+
+	xdbc_delay = xdbc_runtime_delay;
+
+	/*
+	 * It's time to shutdown DbC, so that the debug
+	 * port could be reused by the host controller.
+	 */
+	if (early_xdbc_console.index == -1 ||
+	    (early_xdbc_console.flags & CON_BOOT)) {
+		xdbc_trace("hardware not used any more\n");
+		goto free_and_quit;
+	}
+
+	raw_spin_lock_irqsave(&xdbc.lock, flags);
+	base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
+	if (!base) {
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+		xdbc_trace("failed to remap the io address\n");
+		ret = -ENOMEM;
+		goto free_and_quit;
+	}
+
+	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+	xdbc_trace("early mapped IO address released\n");
+
+	xdbc.xhci_base = base;
+	offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
+	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
+	raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+
+	return 0;
+
+free_and_quit:
+	xdbc_free_ring(&xdbc.evt_ring);
+	xdbc_free_ring(&xdbc.out_ring);
+	xdbc_free_ring(&xdbc.in_ring);
+	free_bootmem(xdbc.table_dma, PAGE_SIZE);
+	free_bootmem(xdbc.out_dma, PAGE_SIZE);
+	writel(0, &xdbc.xdbc_reg->control);
+	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+
+	return ret;
+}
+subsys_initcall(xdbc_init);
diff --git a/drivers/usb/early/xhci-dbc.h b/drivers/usb/early/xhci-dbc.h
new file mode 100644
index 0000000..c46bea2
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.h
@@ -0,0 +1,205 @@
+/*
+ * xhci-dbc.h - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_XHCI_DBC_H
+#define __LINUX_XHCI_DBC_H
+
+#include <linux/types.h>
+#include <linux/usb/ch9.h>
+
+/**
+ * struct xdbc_regs - xHCI Debug Capability Register interface.
+ */
+struct xdbc_regs {
+	__le32	capability;
+	__le32	doorbell;
+	__le32	ersts;		/* Event Ring Segment Table Size*/
+	__le32	rvd0;		/* 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 */
+#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	rvd1;		/* 2b~28 reserved bits */
+	__le64	dccp;		/* Debug Capability Context Pointer */
+	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
+	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
+};
+
+/*
+ * xHCI Debug Capability data structures
+ */
+struct xdbc_trb {
+	__le32 field[4];
+};
+
+struct xdbc_erst_entry {
+	__le64	seg_addr;
+	__le32	seg_size;
+	__le32	rsvd;
+};
+
+struct xdbc_info_context {
+	__le64	string0;
+	__le64	manufacture;
+	__le64	product;
+	__le64	serial;
+	__le32	length;
+	__le32	rsvdz[7];
+};
+
+struct xdbc_ep_context {
+	__le32	ep_info1;
+	__le32	ep_info2;
+	__le64	deq;
+	__le32	tx_info;
+	__le32	rsvd0[11];
+};
+
+struct xdbc_context {
+	struct xdbc_info_context	info;
+	struct xdbc_ep_context		out;
+	struct xdbc_ep_context		in;
+};
+
+#define	XDBC_INFO_CONTEXT_SIZE		48
+
+#define	XDBC_MAX_STRING_LENGTH		64
+#define	XDBC_STRING_MANUFACTURE		"Linux"
+#define	XDBC_STRING_PRODUCT		"Remote GDB"
+#define	XDBC_STRING_SERIAL		"0001"
+struct xdbc_strings {
+	char	string0[XDBC_MAX_STRING_LENGTH];
+	char	manufacture[XDBC_MAX_STRING_LENGTH];
+	char	product[XDBC_MAX_STRING_LENGTH];
+	char	serial[XDBC_MAX_STRING_LENGTH];
+};
+
+#define	XDBC_PROTOCOL		1	/* GNU Remote Debug Command Set */
+#define	XDBC_VENDOR_ID		0x1d6b	/* Linux Foundation 0x1d6b */
+#define	XDBC_PRODUCT_ID		0x0004	/* __le16 idProduct; device 0004 */
+#define	XDBC_DEVICE_REV		0x0010	/* 0.10 */
+
+/*
+ * software state structure
+ */
+struct xdbc_segment {
+	struct xdbc_trb		*trbs;
+	dma_addr_t		dma;
+};
+
+#define	XDBC_TRBS_PER_SEGMENT	256
+
+struct xdbc_ring {
+	struct xdbc_segment	*segment;
+	struct xdbc_trb		*enqueue;
+	struct xdbc_trb		*dequeue;
+	u32			cycle_state;
+};
+
+#define	XDBC_EPID_OUT	2
+#define	XDBC_EPID_IN	3
+
+struct xdbc_state {
+	/* pci device info*/
+	u16		vendor;
+	u16		device;
+	u32		bus;
+	u32		dev;
+	u32		func;
+	void __iomem	*xhci_base;
+	u64		xhci_start;
+	size_t		xhci_length;
+	int		port_number;
+#define	XDBC_PCI_MAX_BUSES		256
+#define	XDBC_PCI_MAX_DEVICES		32
+#define	XDBC_PCI_MAX_FUNCTION		8
+
+	/* DbC register base */
+	struct		xdbc_regs __iomem *xdbc_reg;
+
+	/* DbC table page */
+	dma_addr_t	table_dma;
+	void		*table_base;
+
+#define	XDBC_TABLE_ENTRY_SIZE		64
+#define	XDBC_ERST_ENTRY_NUM		1
+#define	XDBC_DBCC_ENTRY_NUM		3
+#define	XDBC_STRING_ENTRY_NUM		4
+
+	/* event ring segment table */
+	dma_addr_t	erst_dma;
+	size_t		erst_size;
+	void		*erst_base;
+
+	/* event ring segments */
+	struct xdbc_ring	evt_ring;
+	struct xdbc_segment	evt_seg;
+
+	/* debug capability contexts */
+	dma_addr_t	dbcc_dma;
+	size_t		dbcc_size;
+	void		*dbcc_base;
+
+	/* descriptor strings */
+	dma_addr_t	string_dma;
+	size_t		string_size;
+	void		*string_base;
+
+	/* bulk OUT endpoint */
+	struct xdbc_ring	out_ring;
+	struct xdbc_segment	out_seg;
+	void			*out_buf;
+	dma_addr_t		out_dma;
+
+	/* bulk IN endpoint */
+	struct xdbc_ring	in_ring;
+	struct xdbc_segment	in_seg;
+	void			*in_buf;
+	dma_addr_t		in_dma;
+
+	u32			flags;
+#define XDBC_FLAGS_INITIALIZED		BIT(0)
+#define XDBC_FLAGS_BULKIN_STALL		BIT(1)
+#define XDBC_FLAGS_BULKOUT_STALL	BIT(2)
+#define XDBC_FLAGS_IN_PROCESS		BIT(3)
+#define XDBC_FLAGS_OUT_PROCESS		BIT(4)
+#define XDBC_FLAGS_CONFIGURED		BIT(5)
+
+	/* spinlock for early_xdbc_write() reentrancy */
+	raw_spinlock_t		lock;
+};
+
+#define	XDBC_MAX_PACKET		1024
+
+/* door bell target */
+#define	OUT_EP_DOORBELL		0
+#define	IN_EP_DOORBELL		1
+#define	DOOR_BELL_TARGET(p)	(((p) & 0xff) << 8)
+
+#define	xdbc_read64(regs)	xhci_read_64(NULL, (regs))
+#define	xdbc_write64(val, regs)	xhci_write_64(NULL, (val), (regs))
+
+#endif /* __LINUX_XHCI_DBC_H */
diff --git a/include/linux/usb/xhci-dbgp.h b/include/linux/usb/xhci-dbgp.h
new file mode 100644
index 0000000..aa9441f
--- /dev/null
+++ b/include/linux/usb/xhci-dbgp.h
@@ -0,0 +1,22 @@
+/*
+ * Standalone xHCI debug capability driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_XHCI_DBGP_H
+#define __LINUX_XHCI_DBGP_H
+
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+int __init early_xdbc_parse_parameter(char *s);
+int __init early_xdbc_setup_hardware(void);
+void __init early_xdbc_register_console(void);
+#endif /* CONFIG_EARLY_PRINTK_XDBC */
+
+#endif /* __LINUX_XHCI_DBGP_H */
-- 
2.1.4

[PATCH v5 2/4] x86: add support for earlyprintk via USB3 debug port

[Lu Baolu]

Add support for early printk by writing debug messages to the
USB3 debug port.   Users can use this type of early printk by
specifying kernel parameter of "earlyprintk=xdbc". This gives
users a chance of providing debug output.

The hardware for USB3 debug port requires DMA memory blocks.
This requires to delay setting up debugging hardware and
registering boot console until the memblocks are filled.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 Documentation/kernel-parameters.txt | 1 +
 arch/x86/kernel/early_printk.c      | 5 +++++
 arch/x86/kernel/setup.c             | 7 +++++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 37babf9..99b64b3 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1178,6 +1178,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			earlyprintk=ttySn[,baudrate]
 			earlyprintk=dbgp[debugController#]
 			earlyprintk=pciserial,bus:device.function[,baudrate]
+			earlyprintk=xdbc[xhciController#]
 
 			earlyprintk is useful when the kernel crashes before
 			the normal console is initialized. It is not enabled by
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 8a12199..c4031b9 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -17,6 +17,7 @@
 #include <asm/intel-mid.h>
 #include <asm/pgtable.h>
 #include <linux/usb/ehci_def.h>
+#include <linux/usb/xhci-dbgp.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
 #include <asm/pci_x86.h>
@@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)
 		if (!strncmp(buf, "efi", 3))
 			early_console_register(&early_efi_console, keep);
 #endif
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+		if (!strncmp(buf, "xdbc", 4))
+			early_xdbc_parse_parameter(buf + 4);
+#endif
 
 		buf++;
 	}
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9c337b0..09d4a56 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -70,6 +70,8 @@
 #include <linux/tboot.h>
 #include <linux/jiffies.h>
 
+#include <linux/usb/xhci-dbgp.h>
+
 #include <video/edid.h>
 
 #include <asm/mtrr.h>
@@ -1096,6 +1098,11 @@ void __init setup_arch(char **cmdline_p)
 	memblock_set_current_limit(ISA_END_ADDRESS);
 	memblock_x86_fill();
 
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+	if (!early_xdbc_setup_hardware())
+		early_xdbc_register_console();
+#endif
+
 	reserve_bios_regions();
 
 	if (efi_enabled(EFI_MEMMAP)) {
-- 
2.1.4

[PATCH v5 3/4] usb: serial: usb_debug: add support for dbc debug device

[Lu Baolu]

This patch add dbc debug device support in usb_debug driver.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Acked-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/usb_debug.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/usb_debug.c b/drivers/usb/serial/usb_debug.c
index ca2fa5b..92f7e5c 100644
--- a/drivers/usb/serial/usb_debug.c
+++ b/drivers/usb/serial/usb_debug.c
@@ -32,7 +32,18 @@ static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x0525, 0x127a) },
 	{ },
 };
-MODULE_DEVICE_TABLE(usb, id_table);
+
+static const struct usb_device_id dbc_id_table[] = {
+	{ USB_DEVICE(0x1d6b, 0x0004) },
+	{ },
+};
+
+static const struct usb_device_id id_table_combined[] = {
+	{ USB_DEVICE(0x0525, 0x127a) },
+	{ USB_DEVICE(0x1d6b, 0x0004) },
+	{ },
+};
+MODULE_DEVICE_TABLE(usb, id_table_combined);
 
 /* This HW really does not support a serial break, so one will be
  * emulated when ever the break state is set to true.
@@ -71,9 +82,20 @@ static struct usb_serial_driver debug_device = {
 	.process_read_urb =	usb_debug_process_read_urb,
 };
 
+static struct usb_serial_driver dbc_device = {
+	.driver = {
+		.owner =	THIS_MODULE,
+		.name =		"xhci_dbc",
+	},
+	.id_table =		dbc_id_table,
+	.num_ports =		1,
+	.break_ctl =		usb_debug_break_ctl,
+	.process_read_urb =	usb_debug_process_read_urb,
+};
+
 static struct usb_serial_driver * const serial_drivers[] = {
-	&debug_device, NULL
+	&debug_device, &dbc_device, NULL
 };
 
-module_usb_serial_driver(serial_drivers, id_table);
+module_usb_serial_driver(serial_drivers, id_table_combined);
 MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH v5 4/4] usb: doc: add document for USB3 debug port usage

[Lu Baolu]

Add Documentation/usb/usb3-debug-port.rst. This document includes
the user guide for USB3 debug port.

Cc: linux-doc@vger.kernel.org
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 Documentation/usb/usb3-debug-port.rst | 95 +++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/usb/usb3-debug-port.rst

diff --git a/Documentation/usb/usb3-debug-port.rst b/Documentation/usb/usb3-debug-port.rst
new file mode 100644
index 0000000..70eabe4
--- /dev/null
+++ b/Documentation/usb/usb3-debug-port.rst
@@ -0,0 +1,95 @@
+===============
+USB3 debug port
+===============
+
+:Author: Lu Baolu <baolu.lu@linux.intel.com>
+:Date: October 2016
+
+GENERAL
+=======
+
+This is a HOWTO for using USB3 debug port on x86 systems.
+
+Before using any kernel debugging functionalities based on USB3
+debug port, you need to check 1) whether debug port is supported
+by the xHCI host, 2) which port is used for debugging purpose
+(normally the first USB3 root port). You must have a USB 3.0
+super-speed A-to-A debugging cable to connect the debug target
+with a debug host. In this document, a debug target stands for
+the system under debugging; while, a debug host stands for a
+stand-alone system that is able to talk to the debugging target
+through the USB3 debug port.
+
+EARLY PRINTK
+============
+
+On debug target system, you need to customize a debugging kernel
+with CONFIG_EARLY_PRINTK_XDBC enabled. And add below kernel boot
+parameter::
+
+	"earlyprintk=xdbc"
+
+If there are multiple xHCI controllers in the system, you can
+append a host contoller index to this kernel parameter. This
+index is started from 0.
+
+If you are going to leverage the keep option defined by the
+early printk framework to keep the boot console alive after
+early boot, you'd better add below kernel boot parameter::
+
+	"usbcore.autosuspend=-1"
+
+On debug host side, you don't need to customize the kernel, but
+you need to disable usb subsystem runtime power management by
+adding below kernel boot parameter::
+
+	"usbcore.autosuspend=-1"
+
+Before starting the debug target, you should connect the debug
+port on debug target with a root port or port of any external hub
+on the debug host. The cable used to connect these two ports
+should be a USB 3.0 super-speed A-to-A debugging cable.
+
+During early boot of debug target, DbC (the debug engine for USB3
+debug port) hardware gets initialized. Debug host should be able
+to enumerate the debug target as a debug device. Debug host will
+then bind the debug device with the usb_debug driver module and
+create the /dev/ttyUSB0 device.
+
+If device enumeration goes smoothly, you should be able to see
+below kernel messages on debug host::
+
+	# tail -f /var/log/kern.log
+	[ 1815.983374] usb 4-3: new SuperSpeed USB device number 4 using xhci_hcd
+	[ 1815.999595] usb 4-3: LPM exit latency is zeroed, disabling LPM.
+	[ 1815.999899] usb 4-3: New USB device found, idVendor=1d6b, idProduct=0004
+	[ 1815.999902] usb 4-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
+	[ 1815.999903] usb 4-3: Product: Remote GDB
+	[ 1815.999904] usb 4-3: Manufacturer: Linux
+	[ 1815.999905] usb 4-3: SerialNumber: 0001
+	[ 1816.000240] usb_debug 4-3:1.0: xhci_dbc converter detected
+	[ 1816.000360] usb 4-3: xhci_dbc converter now attached to ttyUSB0
+
+You can run below bash scripts on debug host to read the kernel
+log sent from debug target.
+
+.. code-block:: sh
+
+	===== start of bash scripts =============
+	#!/bin/bash
+
+	while true ; do
+		while [ ! -d /sys/class/tty/ttyUSB0 ] ; do
+			:
+	done
+	cat /dev/ttyUSB0 >> xdbc.log
+	done
+	===== end of bash scripts ===============
+
+You should be able to see the early boot message in xdbc.log.
+
+If it doesn't work, please ask it on the <linux-usb@vger.kernel.org>
+mailing list. Below USB hosts have been verified to work::
+
+	Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller
+	Intel Corporation Wildcat Point-LP USB xHCI Controller
-- 
2.1.4

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Lu Baolu]

Hi Greg,

This patch series has been there for 2 months without
further comments. Will you consider it for usb-next?

Best regards,
Lu Baolu

On 11/15/2016 02:02 PM, Lu Baolu wrote:
> xHCI debug capability (DbC) is an optional but standalone
> functionality provided by an xHCI host controller. With DbC
> hardware initialized, the system will present a debug device
> through the USB3 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 set adds support for early printk functionality
> through a USB3 debug port by 1) initializing and enabling
> the DbC hardware during early boot; 2) registering a boot
> console to the system so that early printk messages can go
> through the USB3 debug port. It also includes some lines
> of changes in usb_debug driver so that it can be bound when
> a USB3 debug device is enumerated.
>
> This code is designed to be used only for kernel debugging
> when machine crashes very early before the console code is
> initialized. It makes the life of kernel debugging easier
> when people work with a modern machine without any legacy
> serial ports.
>
> ---
> Change log:
> v4->v5:
>   - add raw_spin_lock to make xdbc_bulk_write() reentrant. 
>
> v3->v4:
>   - Rename the document with .dst suffix.
>   - Add the list of hardware that has been succesfuly
>     tested on in the document.
>
> v2->v3:
>   - Removed spinlock usage.
>   - Removed work queue usage.
>   - Refined the user guide document.
>
> v1->v2:
>   - Refactor the duplicate code in xdbc_early_start() and
>     xdbc_handle_external_reset().
>   - Free resources when hardware not used any more.
>   - Refine the user guide document.
>
> Lu Baolu (4):
>   usb: dbc: early driver for xhci debug capability
>   x86: add support for earlyprintk via USB3 debug port
>   usb: serial: usb_debug: add support for dbc debug device
>   usb: doc: add document for USB3 debug port usage
>
>  Documentation/kernel-parameters.txt   |    1 +
>  Documentation/usb/usb3-debug-port.rst |   95 +++
>  arch/x86/Kconfig.debug                |   14 +
>  arch/x86/kernel/early_printk.c        |    5 +
>  arch/x86/kernel/setup.c               |    7 +
>  drivers/usb/Kconfig                   |    3 +
>  drivers/usb/Makefile                  |    2 +-
>  drivers/usb/early/Makefile            |    1 +
>  drivers/usb/early/xhci-dbc.c          | 1068 +++++++++++++++++++++++++++++++++
>  drivers/usb/early/xhci-dbc.h          |  205 +++++++
>  drivers/usb/serial/usb_debug.c        |   28 +-
>  include/linux/usb/xhci-dbgp.h         |   22 +
>  12 files changed, 1447 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/usb/usb3-debug-port.rst
>  create mode 100644 drivers/usb/early/xhci-dbc.c
>  create mode 100644 drivers/usb/early/xhci-dbc.h
>  create mode 100644 include/linux/usb/xhci-dbgp.h
>

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Greg Kroah-Hartman]

On Wed, Jan 18, 2017 at 02:20:30PM +0800, Lu Baolu wrote:
> Hi Greg,
> 
> This patch series has been there for 2 months without
> further comments. Will you consider it for usb-next?

I needed acks from the x86 maintainers before I could take those
patches.  Can you resend the series and hopefully get them?

thanks,

greg k-h

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Ingo Molnar]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Jan 18, 2017 at 02:20:30PM +0800, Lu Baolu wrote:
> > Hi Greg,
> > 
> > This patch series has been there for 2 months without
> > further comments. Will you consider it for usb-next?
> 
> I needed acks from the x86 maintainers before I could take those
> patches.  Can you resend the series and hopefully get them?

Thanks for the reminder - the patches look mostly good to me, I have a few very 
minor observations, I'll reply to the patches.

Thanks,

	Ingo

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> xHCI debug capability (DbC) is an optional but standalone
> functionality provided by an xHCI host controller. With DbC
> hardware initialized, the system will present a debug device
> through the USB3 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 set adds support for early printk functionality
> through a USB3 debug port by 1) initializing and enabling
> the DbC hardware during early boot; 2) registering a boot
> console to the system so that early printk messages can go
> through the USB3 debug port. It also includes some lines
> of changes in usb_debug driver so that it can be bound when
> a USB3 debug device is enumerated.
> 
> This code is designed to be used only for kernel debugging
> when machine crashes very early before the console code is
> initialized. It makes the life of kernel debugging easier
> when people work with a modern machine without any legacy
> serial ports.

BTW., just a side note, some kernel developers (like PeterZ - and I do it 
sometimes too) remap early_printk to printk permanently and use it as their main 
printk facility - because printk() reliability has suffered over the last couple 
of years.

So it's more than just early boot debugging - it's a very simple state-less 
logging facility to an external computer.


Thanks,

	ngo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> 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.
> 
> This code is designed to be only used for kernel debugging
> when machine crashes very early before the console code is
> initialized. For normal operation it is not recommended.
> 
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  arch/x86/Kconfig.debug        |   14 +
>  drivers/usb/Kconfig           |    3 +
>  drivers/usb/Makefile          |    2 +-
>  drivers/usb/early/Makefile    |    1 +
>  drivers/usb/early/xhci-dbc.c  | 1068 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/early/xhci-dbc.h  |  205 ++++++++
>  include/linux/usb/xhci-dbgp.h |   22 +
>  7 files changed, 1314 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/early/xhci-dbc.c
>  create mode 100644 drivers/usb/early/xhci-dbc.h
>  create mode 100644 include/linux/usb/xhci-dbgp.h
> 
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 67eec55..13e85b7 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -29,6 +29,7 @@ config EARLY_PRINTK
>  config EARLY_PRINTK_DBGP
>  	bool "Early printk via EHCI debug port"
>  	depends on EARLY_PRINTK && PCI
> +	select USB_EARLY_PRINTK
>  	---help---
>  	  Write kernel log output directly into the EHCI debug port.
>  
> @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
>  	  This is useful for kernel debugging when your machine crashes very
>  	  early before the console code is initialized.
>  
> +config EARLY_PRINTK_XDBC
> +	bool "Early printk via xHCI debug port"
> +	depends on EARLY_PRINTK && PCI
> +	select USB_EARLY_PRINTK
> +	---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.

Could we please do this rename:

 s/EARLY_PRINTK_XDBC
   EARLY_PRINTK_USB_XDBC

?

As many people will not realize what 'xdbc' means, standalone - while "it's an 
USB serial logging variant" is a lot more natural.


> +config USB_EARLY_PRINTK
> +	bool

Also, could we standardize the nomencalture to not be a mixture of prefixes and 
postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig space) 
and rename this one to EARLY_PRINTK_USB or so?

You can see the prefix/postfix inconsistency here already:

> -obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
> +obj-$(CONFIG_USB_EARLY_PRINTK)	+= early/
> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o

> +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);
> +	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;
> +	}

> +	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +	    PCI_BASE_ADDRESS_MEM_TYPE_64) {

Please don't break the line here.

> +		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);
> +		mask64 |= ((u64)~0 << 32);

Unnecessary parentheses.

> +	}
> +
> +	sz64 &= mask64;
> +
> +	if (sizeof(dma_addr_t) < 8 || !sz64) {
> +		pr_notice("invalid mmio address\n");
> +		return NULL;
> +	}

So this doesn't work on regular 32-bit kernels?

> +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);

Please no ugly linebreaks.

> +static void xdbc_runtime_delay(unsigned long count)
> +{
> +	udelay(count);
> +}

> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

Is this udelay() complication really necessary? udelay() should work fine even in 
early code. It might not be precisely calibrated, but should be good enough.

> +static int handshake(void __iomem *ptr, u32 mask, u32 done,
> +		     int wait, int delay)

Please break lines more intelligently:

static int
handshake(void __iomem *ptr, u32 mask, u32 done, int wait, int delay)

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

No ugly linebreaks please. There's a ton more in other parts of this patch and 
other patches: please review all the other linebreaks (and ignore checkpatch.pl).

For example this:

> +	xdbc.erst_base = xdbc.table_base +
> +			index * XDBC_TABLE_ENTRY_SIZE;
> +	xdbc.erst_dma = xdbc.table_dma +
> +			index * XDBC_TABLE_ENTRY_SIZE;

should be:

	xdbc.erst_base = xdbc.table_base + index*XDBC_TABLE_ENTRY_SIZE;
	xdbc.erst_dma  = xdbc.table_dma  + index*XDBC_TABLE_ENTRY_SIZE;

which makes it much more readable, etc.

> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> +{
> +	int chunk, ret;
> +	static char buf[XDBC_MAX_PACKET];
> +	int use_cr = 0;
> +
> +	if (!xdbc.xdbc_reg)
> +		return;
> +	memset(buf, 0, XDBC_MAX_PACKET);
> +	while (n > 0) {
> +		for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
> +				str++, chunk++, n--) {
> +			if (!use_cr && *str == '\n') {
> +				use_cr = 1;
> +				buf[chunk] = '\r';
> +				str--;
> +				n++;
> +				continue;
> +			}
> +			if (use_cr)
> +				use_cr = 0;
> +			buf[chunk] = *str;

Hm, why are newlines converted to \r\n unconditionally? Makes for a crappy minicom 
log on the other side ...

> +static int __init xdbc_init(void)
> +{
> +	unsigned long flags;
> +	void __iomem *base;
> +	u32 offset;
> +	int ret = 0;
> +
> +	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
> +		return 0;
> +
> +	xdbc_delay = xdbc_runtime_delay;
> +
> +	/*
> +	 * It's time to shutdown DbC, so that the debug
> +	 * port could be reused by the host controller.

s/shutdown DbC
 /shut down the DbC

s/could be reused
 /can be reused

?

> +	 */
> +	if (early_xdbc_console.index == -1 ||
> +	    (early_xdbc_console.flags & CON_BOOT)) {
> +		xdbc_trace("hardware not used any more\n");

s/any more
  anymore

> +	raw_spin_lock_irqsave(&xdbc.lock, flags);
> +	base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);

Ugh, ioremap() can sleep ...

> +/**
> + * struct xdbc_regs - xHCI Debug Capability Register interface.
> + */
> +struct xdbc_regs {
> +	__le32	capability;
> +	__le32	doorbell;
> +	__le32	ersts;		/* Event Ring Segment Table Size*/
> +	__le32	rvd0;		/* 0c~0f reserved bits */

Yeah, so thsbbrvtnssck. (these abbreviations suck)

Why 'rvd0' - did we run out of letters? Please name it __reserved_0 and 
__reserved_1 like we typically do in kernel code.

> +	__le32	rsvd;

> +	__le32	rsvdz[7];

> +	__le32	rsvd0[11];

ditto.

> +#define	XDBC_INFO_CONTEXT_SIZE		48
> +
> +#define	XDBC_MAX_STRING_LENGTH		64
> +#define	XDBC_STRING_MANUFACTURE		"Linux"
> +#define	XDBC_STRING_PRODUCT		"Remote GDB"
> +#define	XDBC_STRING_SERIAL		"0001"
> +struct xdbc_strings {

Please put a newline between different types of definitions.

> +	char	string0[XDBC_MAX_STRING_LENGTH];
> +	char	manufacture[XDBC_MAX_STRING_LENGTH];
> +	char	product[XDBC_MAX_STRING_LENGTH];
> +	char	serial[XDBC_MAX_STRING_LENGTH];

s/manufacture/manufacturer

?

> +};
> +
> +#define	XDBC_PROTOCOL		1	/* GNU Remote Debug Command Set */
> +#define	XDBC_VENDOR_ID		0x1d6b	/* Linux Foundation 0x1d6b */
> +#define	XDBC_PRODUCT_ID		0x0004	/* __le16 idProduct; device 0004 */
> +#define	XDBC_DEVICE_REV		0x0010	/* 0.10 */
> +
> +/*
> + * software state structure
> + */
> +struct xdbc_segment {
> +	struct xdbc_trb		*trbs;
> +	dma_addr_t		dma;
> +};
> +
> +#define	XDBC_TRBS_PER_SEGMENT	256
> +
> +struct xdbc_ring {
> +	struct xdbc_segment	*segment;
> +	struct xdbc_trb		*enqueue;
> +	struct xdbc_trb		*dequeue;
> +	u32			cycle_state;
> +};
> +
> +#define	XDBC_EPID_OUT	2
> +#define	XDBC_EPID_IN	3
> +
> +struct xdbc_state {
> +	/* pci device info*/
> +	u16		vendor;
> +	u16		device;
> +	u32		bus;
> +	u32		dev;
> +	u32		func;
> +	void __iomem	*xhci_base;
> +	u64		xhci_start;
> +	size_t		xhci_length;
> +	int		port_number;
> +#define	XDBC_PCI_MAX_BUSES		256
> +#define	XDBC_PCI_MAX_DEVICES		32
> +#define	XDBC_PCI_MAX_FUNCTION		8
> +
> +	/* DbC register base */
> +	struct		xdbc_regs __iomem *xdbc_reg;
> +
> +	/* DbC table page */
> +	dma_addr_t	table_dma;
> +	void		*table_base;
> +
> +#define	XDBC_TABLE_ENTRY_SIZE		64
> +#define	XDBC_ERST_ENTRY_NUM		1
> +#define	XDBC_DBCC_ENTRY_NUM		3
> +#define	XDBC_STRING_ENTRY_NUM		4
> +
> +	/* event ring segment table */
> +	dma_addr_t	erst_dma;
> +	size_t		erst_size;
> +	void		*erst_base;
> +
> +	/* event ring segments */
> +	struct xdbc_ring	evt_ring;
> +	struct xdbc_segment	evt_seg;
> +
> +	/* debug capability contexts */
> +	dma_addr_t	dbcc_dma;
> +	size_t		dbcc_size;
> +	void		*dbcc_base;
> +
> +	/* descriptor strings */
> +	dma_addr_t	string_dma;
> +	size_t		string_size;
> +	void		*string_base;
> +
> +	/* bulk OUT endpoint */
> +	struct xdbc_ring	out_ring;
> +	struct xdbc_segment	out_seg;
> +	void			*out_buf;
> +	dma_addr_t		out_dma;
> +
> +	/* bulk IN endpoint */
> +	struct xdbc_ring	in_ring;
> +	struct xdbc_segment	in_seg;
> +	void			*in_buf;
> +	dma_addr_t		in_dma;

Please make the vertical tabulation of the fields consistent throughout the 
structure. Look at it in a terminal and convince yourself that it's nice and 
beautiful to look at!

Also, if you mix CPP #defines into structure definitions then tabulate them in a 
similar fashion.

Thanks,

	Ingo

Re: [PATCH v5 2/4] x86: add support for earlyprintk via USB3 debug port

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> index 8a12199..c4031b9 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -17,6 +17,7 @@
>  #include <asm/intel-mid.h>
>  #include <asm/pgtable.h>
>  #include <linux/usb/ehci_def.h>
> +#include <linux/usb/xhci-dbgp.h>
>  #include <linux/efi.h>
>  #include <asm/efi.h>
>  #include <asm/pci_x86.h>
> @@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)
>  		if (!strncmp(buf, "efi", 3))
>  			early_console_register(&early_efi_console, keep);
>  #endif
> +#ifdef CONFIG_EARLY_PRINTK_XDBC
> +		if (!strncmp(buf, "xdbc", 4))
> +			early_xdbc_parse_parameter(buf + 4);
> +#endif
>  
>  		buf++;
>  	}
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 9c337b0..09d4a56 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -70,6 +70,8 @@
>  #include <linux/tboot.h>
>  #include <linux/jiffies.h>
>  
> +#include <linux/usb/xhci-dbgp.h>
> +
>  #include <video/edid.h>
>  
>  #include <asm/mtrr.h>
> @@ -1096,6 +1098,11 @@ void __init setup_arch(char **cmdline_p)
>  	memblock_set_current_limit(ISA_END_ADDRESS);
>  	memblock_x86_fill();
>  
> +#ifdef CONFIG_EARLY_PRINTK_XDBC
> +	if (!early_xdbc_setup_hardware())
> +		early_xdbc_register_console();
> +#endif
> +
>  	reserve_bios_regions();
>  
>  	if (efi_enabled(EFI_MEMMAP)) {
> -- 
> 2.1.4

Please create proper wrappers in xhci-dbgp.h to not litter generic code with these
#ifdefs.

Thanks,

	Ingo

Re: [PATCH v5 3/4] usb: serial: usb_debug: add support for dbc debug device

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> This patch add dbc debug device support in usb_debug driver.

s/add dbc debug device support in usb_debug driver
 /adds dbc debug device support to the usb_debug driver

Please fix the title as well.

Thanks,

	Ingo

Re: [PATCH v5 4/4] usb: doc: add document for USB3 debug port usage

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Add Documentation/usb/usb3-debug-port.rst. This document includes
> the user guide for USB3 debug port.
> 
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  Documentation/usb/usb3-debug-port.rst | 95 +++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/usb/usb3-debug-port.rst
> 
> diff --git a/Documentation/usb/usb3-debug-port.rst b/Documentation/usb/usb3-debug-port.rst
> new file mode 100644
> index 0000000..70eabe4
> --- /dev/null
> +++ b/Documentation/usb/usb3-debug-port.rst
> @@ -0,0 +1,95 @@
> +===============
> +USB3 debug port
> +===============
> +
> +:Author: Lu Baolu <baolu.lu@linux.intel.com>
> +:Date: October 2016
> +
> +GENERAL
> +=======
> +
> +This is a HOWTO for using USB3 debug port on x86 systems.
> +
> +Before using any kernel debugging functionalities based on USB3

s/debugging functionalities
 /debugging functionality

> +debug port, you need to check 1) whether debug port is supported
> +by the xHCI host, 2) which port is used for debugging purpose

s/debugging purpose
 /debugging purposes

> +On debug target system, you need to customize a debugging kernel

s/On debug target system
  On the debug target system

(There are more typos/grammar errors in the document, please re-read it.)

Thanks,

	Ingo

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Mathias Nyman]

On 19.01.2017 11:09, Ingo Molnar wrote:
>
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
>> On Wed, Jan 18, 2017 at 02:20:30PM +0800, Lu Baolu wrote:
>>> Hi Greg,
>>>
>>> This patch series has been there for 2 months without
>>> further comments. Will you consider it for usb-next?
>>
>> I needed acks from the x86 maintainers before I could take those
>> patches.  Can you resend the series and hopefully get them?
>
> Thanks for the reminder - the patches look mostly good to me, I have a few very
> minor observations, I'll reply to the patches.
>

I have no further comments from xhci pov, If you send a new series fixing those
minor observations I'll ack it

-Mathias

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

I'm very appreciated for your review comments. I've put my
replies in lines.

On 01/19/2017 05:37 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> 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.
>>
>> This code is designed to be only used for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. For normal operation it is not recommended.
>>
>> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>  arch/x86/Kconfig.debug        |   14 +
>>  drivers/usb/Kconfig           |    3 +
>>  drivers/usb/Makefile          |    2 +-
>>  drivers/usb/early/Makefile    |    1 +
>>  drivers/usb/early/xhci-dbc.c  | 1068 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/early/xhci-dbc.h  |  205 ++++++++
>>  include/linux/usb/xhci-dbgp.h |   22 +
>>  7 files changed, 1314 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/usb/early/xhci-dbc.c
>>  create mode 100644 drivers/usb/early/xhci-dbc.h
>>  create mode 100644 include/linux/usb/xhci-dbgp.h
>>
>> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
>> index 67eec55..13e85b7 100644
>> --- a/arch/x86/Kconfig.debug
>> +++ b/arch/x86/Kconfig.debug
>> @@ -29,6 +29,7 @@ config EARLY_PRINTK
>>  config EARLY_PRINTK_DBGP
>>  	bool "Early printk via EHCI debug port"
>>  	depends on EARLY_PRINTK && PCI
>> +	select USB_EARLY_PRINTK
>>  	---help---
>>  	  Write kernel log output directly into the EHCI debug port.
>>  
>> @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
>>  	  This is useful for kernel debugging when your machine crashes very
>>  	  early before the console code is initialized.
>>  
>> +config EARLY_PRINTK_XDBC
>> +	bool "Early printk via xHCI debug port"
>> +	depends on EARLY_PRINTK && PCI
>> +	select USB_EARLY_PRINTK
>> +	---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.
> Could we please do this rename:
>
>  s/EARLY_PRINTK_XDBC
>    EARLY_PRINTK_USB_XDBC
>
> ?
>
> As many people will not realize what 'xdbc' means, standalone - while "it's an 
> USB serial logging variant" is a lot more natural.
>
>
>> +config USB_EARLY_PRINTK
>> +	bool
> Also, could we standardize the nomencalture to not be a mixture of prefixes and 
> postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig space) 
> and rename this one to EARLY_PRINTK_USB or so?
>
> You can see the prefix/postfix inconsistency here already:

Sure. I will fix the names. Thanks.

>
>> -obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
>> +obj-$(CONFIG_USB_EARLY_PRINTK)	+= early/
>> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
>> +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);
>> +	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;
>> +	}
>> +	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
>> +	    PCI_BASE_ADDRESS_MEM_TYPE_64) {
> Please don't break the line here.

Sure. Will fix it.

>
>> +		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);
>> +		mask64 |= ((u64)~0 << 32);
> Unnecessary parentheses.

Sure. Will fix it.

>
>> +	}
>> +
>> +	sz64 &= mask64;
>> +
>> +	if (sizeof(dma_addr_t) < 8 || !sz64) {
>> +		pr_notice("invalid mmio address\n");
>> +		return NULL;
>> +	}
> So this doesn't work on regular 32-bit kernels?

I will run my code on a 32-bit kernel and remove this check
if it passes the test.

>
>> +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);
> Please no ugly linebreaks.

Sorry. I will fix it.

>
>> +static void xdbc_runtime_delay(unsigned long count)
>> +{
>> +	udelay(count);
>> +}
>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> Is this udelay() complication really necessary? udelay() should work fine even in 
> early code. It might not be precisely calibrated, but should be good enough.

I tried udelay() in the early code. It's not precise enough for the
hardware handshaking.

>
>> +static int handshake(void __iomem *ptr, u32 mask, u32 done,
>> +		     int wait, int delay)
> Please break lines more intelligently:
>
> static int
> handshake(void __iomem *ptr, u32 mask, u32 done, int wait, int delay)

Sure. I will fix it.

>
>> +	ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
>> +						0, XHCI_EXT_CAPS_LEGACY);
> No ugly linebreaks please. There's a ton more in other parts of this patch and 
> other patches: please review all the other linebreaks (and ignore checkpatch.pl).
>
> For example this:
>
>> +	xdbc.erst_base = xdbc.table_base +
>> +			index * XDBC_TABLE_ENTRY_SIZE;
>> +	xdbc.erst_dma = xdbc.table_dma +
>> +			index * XDBC_TABLE_ENTRY_SIZE;
> should be:
>
> 	xdbc.erst_base = xdbc.table_base + index*XDBC_TABLE_ENTRY_SIZE;
> 	xdbc.erst_dma  = xdbc.table_dma  + index*XDBC_TABLE_ENTRY_SIZE;
>
> which makes it much more readable, etc.

Sure.
These line breaks were added to make checkpatch.pl happy.
I will review all the line breaks and make them more readable.

>
>> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
>> +{
>> +	int chunk, ret;
>> +	static char buf[XDBC_MAX_PACKET];
>> +	int use_cr = 0;
>> +
>> +	if (!xdbc.xdbc_reg)
>> +		return;
>> +	memset(buf, 0, XDBC_MAX_PACKET);
>> +	while (n > 0) {
>> +		for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
>> +				str++, chunk++, n--) {
>> +			if (!use_cr && *str == '\n') {
>> +				use_cr = 1;
>> +				buf[chunk] = '\r';
>> +				str--;
>> +				n++;
>> +				continue;
>> +			}
>> +			if (use_cr)
>> +				use_cr = 0;
>> +			buf[chunk] = *str;
> Hm, why are newlines converted to \r\n unconditionally? Makes for a crappy minicom 
> log on the other side ...

Yes. The usb ehci (usb2) debug port driver (drivers/usb/early/ehci-dbgp.c)
does this. I kept the same for xhci (usb3). It turns out to be good for display
on host side.

>
>> +static int __init xdbc_init(void)
>> +{
>> +	unsigned long flags;
>> +	void __iomem *base;
>> +	u32 offset;
>> +	int ret = 0;
>> +
>> +	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
>> +		return 0;
>> +
>> +	xdbc_delay = xdbc_runtime_delay;
>> +
>> +	/*
>> +	 * It's time to shutdown DbC, so that the debug
>> +	 * port could be reused by the host controller.
> s/shutdown DbC
>  /shut down the DbC
>
> s/could be reused
>  /can be reused
>
> ?
>

Sure. I will fix it. Thanks.

>> +	 */
>> +	if (early_xdbc_console.index == -1 ||
>> +	    (early_xdbc_console.flags & CON_BOOT)) {
>> +		xdbc_trace("hardware not used any more\n");
> s/any more
>   anymore

Sure. I will fix it. Thanks.

>
>> +	raw_spin_lock_irqsave(&xdbc.lock, flags);
>> +	base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
> Ugh, ioremap() can sleep ...

Oh, right. I will remove the remapping code and let it use the
previously mapped one.

>
>> +/**
>> + * struct xdbc_regs - xHCI Debug Capability Register interface.
>> + */
>> +struct xdbc_regs {
>> +	__le32	capability;
>> +	__le32	doorbell;
>> +	__le32	ersts;		/* Event Ring Segment Table Size*/
>> +	__le32	rvd0;		/* 0c~0f reserved bits */
> Yeah, so thsbbrvtnssck. (these abbreviations suck)
>
> Why 'rvd0' - did we run out of letters? Please name it __reserved_0 and 
> __reserved_1 like we typically do in kernel code.

Sure. I will fix it. Thanks.

>
>> +	__le32	rsvd;
>> +	__le32	rsvdz[7];
>> +	__le32	rsvd0[11];
> ditto.

I will fix them.

>
>> +#define	XDBC_INFO_CONTEXT_SIZE		48
>> +
>> +#define	XDBC_MAX_STRING_LENGTH		64
>> +#define	XDBC_STRING_MANUFACTURE		"Linux"
>> +#define	XDBC_STRING_PRODUCT		"Remote GDB"
>> +#define	XDBC_STRING_SERIAL		"0001"
>> +struct xdbc_strings {
> Please put a newline between different types of definitions.

Sure.

>
>> +	char	string0[XDBC_MAX_STRING_LENGTH];
>> +	char	manufacture[XDBC_MAX_STRING_LENGTH];
>> +	char	product[XDBC_MAX_STRING_LENGTH];
>> +	char	serial[XDBC_MAX_STRING_LENGTH];
> s/manufacture/manufacturer
>
> ?

Sure.

>
>> +};
>> +
>> +#define	XDBC_PROTOCOL		1	/* GNU Remote Debug Command Set */
>> +#define	XDBC_VENDOR_ID		0x1d6b	/* Linux Foundation 0x1d6b */
>> +#define	XDBC_PRODUCT_ID		0x0004	/* __le16 idProduct; device 0004 */
>> +#define	XDBC_DEVICE_REV		0x0010	/* 0.10 */
>> +
>> +/*
>> + * software state structure
>> + */
>> +struct xdbc_segment {
>> +	struct xdbc_trb		*trbs;
>> +	dma_addr_t		dma;
>> +};
>> +
>> +#define	XDBC_TRBS_PER_SEGMENT	256
>> +
>> +struct xdbc_ring {
>> +	struct xdbc_segment	*segment;
>> +	struct xdbc_trb		*enqueue;
>> +	struct xdbc_trb		*dequeue;
>> +	u32			cycle_state;
>> +};
>> +
>> +#define	XDBC_EPID_OUT	2
>> +#define	XDBC_EPID_IN	3
>> +
>> +struct xdbc_state {
>> +	/* pci device info*/
>> +	u16		vendor;
>> +	u16		device;
>> +	u32		bus;
>> +	u32		dev;
>> +	u32		func;
>> +	void __iomem	*xhci_base;
>> +	u64		xhci_start;
>> +	size_t		xhci_length;
>> +	int		port_number;
>> +#define	XDBC_PCI_MAX_BUSES		256
>> +#define	XDBC_PCI_MAX_DEVICES		32
>> +#define	XDBC_PCI_MAX_FUNCTION		8
>> +
>> +	/* DbC register base */
>> +	struct		xdbc_regs __iomem *xdbc_reg;
>> +
>> +	/* DbC table page */
>> +	dma_addr_t	table_dma;
>> +	void		*table_base;
>> +
>> +#define	XDBC_TABLE_ENTRY_SIZE		64
>> +#define	XDBC_ERST_ENTRY_NUM		1
>> +#define	XDBC_DBCC_ENTRY_NUM		3
>> +#define	XDBC_STRING_ENTRY_NUM		4
>> +
>> +	/* event ring segment table */
>> +	dma_addr_t	erst_dma;
>> +	size_t		erst_size;
>> +	void		*erst_base;
>> +
>> +	/* event ring segments */
>> +	struct xdbc_ring	evt_ring;
>> +	struct xdbc_segment	evt_seg;
>> +
>> +	/* debug capability contexts */
>> +	dma_addr_t	dbcc_dma;
>> +	size_t		dbcc_size;
>> +	void		*dbcc_base;
>> +
>> +	/* descriptor strings */
>> +	dma_addr_t	string_dma;
>> +	size_t		string_size;
>> +	void		*string_base;
>> +
>> +	/* bulk OUT endpoint */
>> +	struct xdbc_ring	out_ring;
>> +	struct xdbc_segment	out_seg;
>> +	void			*out_buf;
>> +	dma_addr_t		out_dma;
>> +
>> +	/* bulk IN endpoint */
>> +	struct xdbc_ring	in_ring;
>> +	struct xdbc_segment	in_seg;
>> +	void			*in_buf;
>> +	dma_addr_t		in_dma;
> Please make the vertical tabulation of the fields consistent throughout the 
> structure. Look at it in a terminal and convince yourself that it's nice and 
> beautiful to look at!
>
> Also, if you mix CPP #defines into structure definitions then tabulate them in a 
> similar fashion.

Sure. I will fix this. Thank you.

Best regards,
Lu Baolu

Re: [PATCH v5 2/4] x86: add support for earlyprintk via USB3 debug port

[Lu Baolu]

Hi Ingo,

On 01/19/2017 05:38 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> index 8a12199..c4031b9 100644
>> --- a/arch/x86/kernel/early_printk.c
>> +++ b/arch/x86/kernel/early_printk.c
>> @@ -17,6 +17,7 @@
>>  #include <asm/intel-mid.h>
>>  #include <asm/pgtable.h>
>>  #include <linux/usb/ehci_def.h>
>> +#include <linux/usb/xhci-dbgp.h>
>>  #include <linux/efi.h>
>>  #include <asm/efi.h>
>>  #include <asm/pci_x86.h>
>> @@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)
>>  		if (!strncmp(buf, "efi", 3))
>>  			early_console_register(&early_efi_console, keep);
>>  #endif
>> +#ifdef CONFIG_EARLY_PRINTK_XDBC
>> +		if (!strncmp(buf, "xdbc", 4))
>> +			early_xdbc_parse_parameter(buf + 4);
>> +#endif
>>  
>>  		buf++;
>>  	}
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 9c337b0..09d4a56 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -70,6 +70,8 @@
>>  #include <linux/tboot.h>
>>  #include <linux/jiffies.h>
>>  
>> +#include <linux/usb/xhci-dbgp.h>
>> +
>>  #include <video/edid.h>
>>  
>>  #include <asm/mtrr.h>
>> @@ -1096,6 +1098,11 @@ void __init setup_arch(char **cmdline_p)
>>  	memblock_set_current_limit(ISA_END_ADDRESS);
>>  	memblock_x86_fill();
>>  
>> +#ifdef CONFIG_EARLY_PRINTK_XDBC
>> +	if (!early_xdbc_setup_hardware())
>> +		early_xdbc_register_console();
>> +#endif
>> +
>>  	reserve_bios_regions();
>>  
>>  	if (efi_enabled(EFI_MEMMAP)) {
>> -- 
>> 2.1.4
> Please create proper wrappers in xhci-dbgp.h to not litter generic code with these
> #ifdefs.

Sure.

Best regards,
Lu Baolu

Re: [PATCH v5 3/4] usb: serial: usb_debug: add support for dbc debug device

[Lu Baolu]

Hi Ingo,

On 01/19/2017 05:39 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> This patch add dbc debug device support in usb_debug driver.
> s/add dbc debug device support in usb_debug driver
>  /adds dbc debug device support to the usb_debug driver
>
> Please fix the title as well.

Sure. Thank you.

Best regards,
Lu Baolu

Re: [PATCH v5 4/4] usb: doc: add document for USB3 debug port usage

[Lu Baolu]

Hi Ingo,

On 01/19/2017 05:41 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Add Documentation/usb/usb3-debug-port.rst. This document includes
>> the user guide for USB3 debug port.
>>
>> Cc: linux-doc@vger.kernel.org
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>  Documentation/usb/usb3-debug-port.rst | 95 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 95 insertions(+)
>>  create mode 100644 Documentation/usb/usb3-debug-port.rst
>>
>> diff --git a/Documentation/usb/usb3-debug-port.rst b/Documentation/usb/usb3-debug-port.rst
>> new file mode 100644
>> index 0000000..70eabe4
>> --- /dev/null
>> +++ b/Documentation/usb/usb3-debug-port.rst
>> @@ -0,0 +1,95 @@
>> +===============
>> +USB3 debug port
>> +===============
>> +
>> +:Author: Lu Baolu <baolu.lu@linux.intel.com>
>> +:Date: October 2016
>> +
>> +GENERAL
>> +=======
>> +
>> +This is a HOWTO for using USB3 debug port on x86 systems.
>> +
>> +Before using any kernel debugging functionalities based on USB3
> s/debugging functionalities
>  /debugging functionality
>
>> +debug port, you need to check 1) whether debug port is supported
>> +by the xHCI host, 2) which port is used for debugging purpose
> s/debugging purpose
>  /debugging purposes
>
>> +On debug target system, you need to customize a debugging kernel
> s/On debug target system
>   On the debug target system
>
> (There are more typos/grammar errors in the document, please re-read it.)

Sure. Thank you.

Best regards,
Lu Baolu

Re: [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port

[Lu Baolu]

Hi Ingo,

On 01/19/2017 05:12 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> xHCI debug capability (DbC) is an optional but standalone
>> functionality provided by an xHCI host controller. With DbC
>> hardware initialized, the system will present a debug device
>> through the USB3 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 set adds support for early printk functionality
>> through a USB3 debug port by 1) initializing and enabling
>> the DbC hardware during early boot; 2) registering a boot
>> console to the system so that early printk messages can go
>> through the USB3 debug port. It also includes some lines
>> of changes in usb_debug driver so that it can be bound when
>> a USB3 debug device is enumerated.
>>
>> This code is designed to be used only for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. It makes the life of kernel debugging easier
>> when people work with a modern machine without any legacy
>> serial ports.
> BTW., just a side note, some kernel developers (like PeterZ - and I do it 
> sometimes too) remap early_printk to printk permanently and use it as their main 
> printk facility - because printk() reliability has suffered over the last couple 
> of years.
>
> So it's more than just early boot debugging - it's a very simple state-less 
> logging facility to an external computer.

Thanks for the information. I will rework this message.

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> >
> >> +static void xdbc_runtime_delay(unsigned long count)
> >> +{
> >> +	udelay(count);
> >> +}
> >> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> > Is this udelay() complication really necessary? udelay() should work fine even in 
> > early code. It might not be precisely calibrated, but should be good enough.
> 
> I tried udelay() in the early code. It's not precise enough for the
> hardware handshaking.

Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
whatsoever.

Could you try it on top of this commit in tip:timers/core:

  4c45c5167c95 x86/timer: Make delay() work during early bootup

?

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

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

Hi Ingo,

On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>>>> +static void xdbc_runtime_delay(unsigned long count)
>>>> +{
>>>> +	udelay(count);
>>>> +}
>>>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
>>> Is this udelay() complication really necessary? udelay() should work fine even in 
>>> early code. It might not be precisely calibrated, but should be good enough.
>> I tried udelay() in the early code. It's not precise enough for the
>> hardware handshaking.
> Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
> whatsoever.

Yes.

>
> Could you try it on top of this commit in tip:timers/core:
>
>   4c45c5167c95 x86/timer: Make delay() work during early bootup
>
> ?

I tried tip:timers/core. It's not precise enough for my context either.

__const_udelay().

157 inline void __const_udelay(unsigned long xloops)
158 {
159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
160         int d0;
161
162         xloops *= 4;
163         asm("mull %%edx"
164                 :"=d" (xloops), "=&a" (d0)
165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
166
167         __delay(++xloops);
168 }


In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
is 4096 (default value).

The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
about 2000 times different.

I did a hacky test in kernel to check the difference between these two different
"lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.

[    2.494751] udelay_test uninitialized ---->start
[    2.494820] udelay_test uninitialized ---->end
[    2.494828] udelay_test initialized ---->start
[    2.595234] udelay_test initialized ---->end

For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
only 69us.

Best regards,
Lu Baolu

[-- Attachment #2: kernel-hack.patch --]
[-- Type: text/x-patch, Size: 2325 bytes --]

diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index a8e91ae..ffc2874 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -168,6 +168,36 @@ inline void __const_udelay(unsigned long xloops)
 }
 EXPORT_SYMBOL(__const_udelay);
 
+void udelay_uninitialized(unsigned long xloops)
+{
+	unsigned long lpj = (1<<12);
+	int d0;
+
+	xloops *= 0x10c7ul;
+	xloops *= 4;
+	asm("mull %%edx"
+		:"=d" (xloops), "=&a" (d0)
+		:"1" (xloops), "0" (lpj * (HZ / 4)));
+
+	delay_loop(++xloops);
+}
+EXPORT_SYMBOL(udelay_uninitialized);
+
+void udelay_initialized(unsigned long xloops)
+{
+	unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy);
+	int d0;
+
+	xloops *= 0x10c7ul;
+	xloops *= 4;
+	asm("mull %%edx"
+		:"=d" (xloops), "=&a" (d0)
+		:"1" (xloops), "0" (lpj * (HZ / 4)));
+
+	delay_loop(++xloops);
+}
+EXPORT_SYMBOL(udelay_initialized);
+
 void __udelay(unsigned long usecs)
 {
 	__const_udelay(usecs * 0x000010c7); /* 2**32 / 1000000 (rounded up) */
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 954abfd..b6a7437 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -302,6 +302,21 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
 	pm_runtime_put_noidle(&dev->dev);
 
+	do {
+		int count = 1000;
+
+		pr_notice("udelay_test uninitialized ---->start\n");
+		while (count-- > 0)
+			udelay_uninitialized(100);
+		pr_notice("udelay_test uninitialized ---->end\n");
+
+		count = 1000;
+		pr_notice("udelay_test initialized ---->start\n");
+		while (count-- > 0)
+			udelay_initialized(100);
+		pr_notice("udelay_test initialized ---->end\n");
+	} while (0);
+
 	return 0;
 
 put_usb3_hcd:
diff --git a/include/asm-generic/delay.h b/include/asm-generic/delay.h
index 0f79054..200ab55 100644
--- a/include/asm-generic/delay.h
+++ b/include/asm-generic/delay.h
@@ -9,6 +9,8 @@ extern void __udelay(unsigned long usecs);
 extern void __ndelay(unsigned long nsecs);
 extern void __const_udelay(unsigned long xloops);
 extern void __delay(unsigned long loops);
+extern void udelay_uninitialized(unsigned long xloops);
+extern void udelay_initialized(unsigned long xloops);
 
 /*
  * The weird n/20000 thing suppresses a "comparison is always false due to

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Ingo,
> 
> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >>>> +static void xdbc_runtime_delay(unsigned long count)
> >>>> +{
> >>>> +	udelay(count);
> >>>> +}
> >>>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> >>> Is this udelay() complication really necessary? udelay() should work fine even in 
> >>> early code. It might not be precisely calibrated, but should be good enough.
> >> I tried udelay() in the early code. It's not precise enough for the
> >> hardware handshaking.
> > Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
> > whatsoever.
> 
> Yes.
> 
> >
> > Could you try it on top of this commit in tip:timers/core:
> >
> >   4c45c5167c95 x86/timer: Make delay() work during early bootup
> >
> > ?
> 
> I tried tip:timers/core. It's not precise enough for my context either.
> 
> __const_udelay().
> 
> 157 inline void __const_udelay(unsigned long xloops)
> 158 {
> 159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
> 160         int d0;
> 161
> 162         xloops *= 4;
> 163         asm("mull %%edx"
> 164                 :"=d" (xloops), "=&a" (d0)
> 165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
> 166
> 167         __delay(++xloops);
> 168 }
> 
> 
> In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
> is 4096 (default value).
> 
> The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
> about 2000 times different.
> 
> I did a hacky test in kernel to check the difference between these two different
> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.
> 
> [    2.494751] udelay_test uninitialized ---->start
> [    2.494820] udelay_test uninitialized ---->end
> [    2.494828] udelay_test initialized ---->start
> [    2.595234] udelay_test initialized ---->end
> 
> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
> only 69us.

Ok, then could we add some simple calibration to make udelay work much better - or 
perhaps move the udelay calibration up earlier?

Hiding essentially an early udelay() implementation in an early-printk driver is 
ugly and counterproductive.

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

On 01/24/2017 04:20 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Ingo,
>>
>> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
>>> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>
>>>>>> +static void xdbc_runtime_delay(unsigned long count)
>>>>>> +{
>>>>>> +	udelay(count);
>>>>>> +}
>>>>>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
>>>>> Is this udelay() complication really necessary? udelay() should work fine even in 
>>>>> early code. It might not be precisely calibrated, but should be good enough.
>>>> I tried udelay() in the early code. It's not precise enough for the
>>>> hardware handshaking.
>>> Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
>>> whatsoever.
>> Yes.
>>
>>> Could you try it on top of this commit in tip:timers/core:
>>>
>>>   4c45c5167c95 x86/timer: Make delay() work during early bootup
>>>
>>> ?
>> I tried tip:timers/core. It's not precise enough for my context either.
>>
>> __const_udelay().
>>
>> 157 inline void __const_udelay(unsigned long xloops)
>> 158 {
>> 159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
>> 160         int d0;
>> 161
>> 162         xloops *= 4;
>> 163         asm("mull %%edx"
>> 164                 :"=d" (xloops), "=&a" (d0)
>> 165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
>> 166
>> 167         __delay(++xloops);
>> 168 }
>>
>>
>> In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
>> is 4096 (default value).
>>
>> The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
>> about 2000 times different.
>>
>> I did a hacky test in kernel to check the difference between these two different
>> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.
>>
>> [    2.494751] udelay_test uninitialized ---->start
>> [    2.494820] udelay_test uninitialized ---->end
>> [    2.494828] udelay_test initialized ---->start
>> [    2.595234] udelay_test initialized ---->end
>>
>> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
>> only 69us.
> Ok, then could we add some simple calibration to make udelay work much better - or 
> perhaps move the udelay calibration up earlier?
>
> Hiding essentially an early udelay() implementation in an early-printk driver is 
> ugly and counterproductive.

Sure. How about below change?

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index d3f0c84..940989e 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
        return size;
 }
 
+static void __init xdbc_udelay_calibration(void)
+{
+       unsigned long lpj = 0;
+       unsigned int tsc_khz, cpu_khz;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               goto calibration_out;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       if (tsc_khz == 0)
+               tsc_khz = cpu_khz;
+       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+               cpu_khz = tsc_khz;
+
+       if (!tsc_khz)
+               goto calibration_out;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+
+calibration_out:
+       if (!lpj)
+               lpj = 1 << 22;
+
+       loops_per_jiffy = lpj;
+}
+
 static int __init xdbc_early_setup(void)
 {
        int ret;
@@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
        }
        xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 
+       xdbc_udelay_calibration();
+
        return 0;
 }

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> > Hiding essentially an early udelay() implementation in an early-printk driver is 
> > ugly and counterproductive.
> 
> Sure. How about below change?
> 
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index d3f0c84..940989e 100644
> --- a/drivers/usb/early/xhci-dbc.c
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
>         return size;
>  }
>  
> +static void __init xdbc_udelay_calibration(void)
> +{
> +       unsigned long lpj = 0;
> +       unsigned int tsc_khz, cpu_khz;
> +
> +       if (!boot_cpu_has(X86_FEATURE_TSC))
> +               goto calibration_out;
> +
> +       cpu_khz = x86_platform.calibrate_cpu();
> +       tsc_khz = x86_platform.calibrate_tsc();
> +
> +       if (tsc_khz == 0)
> +               tsc_khz = cpu_khz;
> +       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
> +               cpu_khz = tsc_khz;
> +
> +       if (!tsc_khz)
> +               goto calibration_out;
> +
> +       lpj = tsc_khz * 1000;
> +       do_div(lpj, HZ);
> +
> +calibration_out:
> +       if (!lpj)
> +               lpj = 1 << 22;
> +
> +       loops_per_jiffy = lpj;
> +}
> +
>  static int __init xdbc_early_setup(void)
>  {
>         int ret;
> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>         }
>         xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
>  
> +       xdbc_udelay_calibration();
> +
>         return 0;
>  }

Yeah - so could we do this in a more generic fashion, not in the early-printk 
driver but in core x86 code?

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> > > Hiding essentially an early udelay() implementation in an early-printk driver is 
> > > ugly and counterproductive.

> Yeah - so could we do this in a more generic fashion, not in the early-printk 
> driver but in core x86 code?

So ideally early_printk() would not depend on udelay() being setup.

In fact, ideally early_printk() wouldn't even use udelay -- this very
much includes its own copy.

Why is udelay() required? Can't the thing simply poll its own register
state to wait for completion?

This all sounds like xdbc cruft is still unreliably garbage..

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 01/25/2017 05:23 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>>> Hiding essentially an early udelay() implementation in an early-printk driver is 
>>> ugly and counterproductive.
>> Sure. How about below change?
>>
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index d3f0c84..940989e 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
>>         return size;
>>  }
>>  
>> +static void __init xdbc_udelay_calibration(void)
>> +{
>> +       unsigned long lpj = 0;
>> +       unsigned int tsc_khz, cpu_khz;
>> +
>> +       if (!boot_cpu_has(X86_FEATURE_TSC))
>> +               goto calibration_out;
>> +
>> +       cpu_khz = x86_platform.calibrate_cpu();
>> +       tsc_khz = x86_platform.calibrate_tsc();
>> +
>> +       if (tsc_khz == 0)
>> +               tsc_khz = cpu_khz;
>> +       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
>> +               cpu_khz = tsc_khz;
>> +
>> +       if (!tsc_khz)
>> +               goto calibration_out;
>> +
>> +       lpj = tsc_khz * 1000;
>> +       do_div(lpj, HZ);
>> +
>> +calibration_out:
>> +       if (!lpj)
>> +               lpj = 1 << 22;
>> +
>> +       loops_per_jiffy = lpj;
>> +}
>> +
>>  static int __init xdbc_early_setup(void)
>>  {
>>         int ret;
>> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>>         }
>>         xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
>>  
>> +       xdbc_udelay_calibration();
>> +
>>         return 0;
>>  }
> Yeah - so could we do this in a more generic fashion, not in the early-printk 
> driver but in core x86 code?
>

Sure. I will move this to arch/x86/kernel/setup.c.

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 01/25/2017 05:57 PM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote:
>> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>>>> Hiding essentially an early udelay() implementation in an early-printk driver is 
>>>> ugly and counterproductive.
>> Yeah - so could we do this in a more generic fashion, not in the early-printk 
>> driver but in core x86 code?
> So ideally early_printk() would not depend on udelay() being setup.
>
> In fact, ideally early_printk() wouldn't even use udelay -- this very
> much includes its own copy.
>
> Why is udelay() required? Can't the thing simply poll its own register
> state to wait for completion?

In my driver, udelay() is mostly used to handle time out.

Xdbc hides most USB things in its firmware. Early printk driver only needs
to setup the registers/data structures and wait until link ready or time out.
Without udelay(), I have no means to convert the polling times into waiting
time.

Best regards,
Lu Baolu

>
> This all sounds like xdbc cruft is still unreliably garbage..
>

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
> In my driver, udelay() is mostly used to handle time out.
> 
> Xdbc hides most USB things in its firmware. Early printk driver only needs
> to setup the registers/data structures and wait until link ready or time out.
> Without udelay(), I have no means to convert the polling times into waiting
> time.

What is timeout and why? If there is an error other than !ready, I would
expect the hardware to inform you of this through another status bit,
no?

So why can't you poll indefinitely for either ready or error?

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 01/25/2017 10:38 PM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
>> In my driver, udelay() is mostly used to handle time out.
>>
>> Xdbc hides most USB things in its firmware. Early printk driver only needs
>> to setup the registers/data structures and wait until link ready or time out.
>> Without udelay(), I have no means to convert the polling times into waiting
>> time.
> What is timeout and why?

Put it in simple:

The driver sets the RUN bit in control register and polls READY
bit in status register for the successful USB device enumeration.
As the USB device enumeration might fail and the READY bit will
never be set, the driver must have a timeout logic to avoid
endless loop.

More details:

The operational model is that driver sets up all necessary registers
and data structures, and then starts the debug engine by setting
the RUN/STOP bit in the control register.

The debug engine then brings up itself as a ready-for-enumeration
USB device. The USB link between host and device starts link training
and then host will detect the connected device. The hub driver in
host will then starts the USB device enumeration processes (as defined
in USB spec). If everything goes smoothly, the device gets enumerated
and host can talk with the debug device.

After that, xdbc firmware will set the READY bit in status register. And
the driver can go ahead with data transfer over USB.

>  If there is an error other than !ready, I would
> expect the hardware to inform you of this through another status bit,
> no?

Yeah, this might be another choice of hardware design. But it's not a
topic for this driver.

>
> So why can't you poll indefinitely for either ready or error?
>
>

Even if the hardware has both ready and error status bits, it's still
nice to have a time out watch dog. Buggy hardware or firmware
might not set any of these bits. Polling indefinitely might result in
a endless loop.

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote:

> > What is timeout and why?
> 
> Put it in simple:
> 
> The driver sets the RUN bit in control register and polls READY
> bit in status register for the successful USB device enumeration.
> As the USB device enumeration might fail and the READY bit will
> never be set, the driver must have a timeout logic to avoid
> endless loop.
> 
> More details:
> 
> The operational model is that driver sets up all necessary registers
> and data structures, and then starts the debug engine by setting
> the RUN/STOP bit in the control register.
> 
> The debug engine then brings up itself as a ready-for-enumeration
> USB device. The USB link between host and device starts link training
> and then host will detect the connected device. The hub driver in
> host will then starts the USB device enumeration processes (as defined
> in USB spec). If everything goes smoothly, the device gets enumerated
> and host can talk with the debug device.
> 
> After that, xdbc firmware will set the READY bit in status register. And
> the driver can go ahead with data transfer over USB.

I have vague memories from a prior discussion where you said this READY
state can be lost at any time (cable unplug or whatnot) and at that
point the driver should re-start the setup, right?

> >  If there is an error other than !ready, I would
> > expect the hardware to inform you of this through another status bit,
> > no?
> 
> Yeah, this might be another choice of hardware design. But it's not a
> topic for this driver.

So is there really no way to way to distinguish between "I did setup and
am waiting for READY", "I did setup, am waiting for READY, but things
got hosed" and "I was READY, things be hosed" ?

I suppose the first and last can be distinguished by remembering if you
ever saw READY, but the first and second are the interesting case I
think.

> > So why can't you poll indefinitely for either ready or error?
> >
> 
> Even if the hardware has both ready and error status bits, it's still
> nice to have a time out watch dog. Buggy hardware or firmware
> might not set any of these bits. Polling indefinitely might result in
> a endless loop.

Loosing output, esp. without indication, is very _very_ annoying when
you're debugging things. Its just about on par with a stuck system, at
least then you know something bad happened.

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

On 01/25/2017 05:23 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>>> Hiding essentially an early udelay() implementation in an early-printk driver is 
>>> ugly and counterproductive.
>> Sure. How about below change?
>>
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index d3f0c84..940989e 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
>>         return size;
>>  }
>>  
>> +static void __init xdbc_udelay_calibration(void)
>> +{
>> +       unsigned long lpj = 0;
>> +       unsigned int tsc_khz, cpu_khz;
>> +
>> +       if (!boot_cpu_has(X86_FEATURE_TSC))
>> +               goto calibration_out;
>> +
>> +       cpu_khz = x86_platform.calibrate_cpu();
>> +       tsc_khz = x86_platform.calibrate_tsc();
>> +
>> +       if (tsc_khz == 0)
>> +               tsc_khz = cpu_khz;
>> +       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
>> +               cpu_khz = tsc_khz;
>> +
>> +       if (!tsc_khz)
>> +               goto calibration_out;
>> +
>> +       lpj = tsc_khz * 1000;
>> +       do_div(lpj, HZ);
>> +
>> +calibration_out:
>> +       if (!lpj)
>> +               lpj = 1 << 22;
>> +
>> +       loops_per_jiffy = lpj;
>> +}
>> +
>>  static int __init xdbc_early_setup(void)
>>  {
>>         int ret;
>> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>>         }
>>         xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
>>  
>> +       xdbc_udelay_calibration();
>> +
>>         return 0;
>>  }
> Yeah - so could we do this in a more generic fashion, not in the early-printk 
> driver but in core x86 code?

How about below changes?

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4cfba94..aab7faa 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -835,6 +835,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
        return 0;
 }
 
+static void __init simple_udelay_calibration(void)
+{
+       unsigned int tsc_khz, cpu_khz;
+       unsigned long lpj;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               return;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       tsc_khz = tsc_khz ? : cpu_khz;
+       if (!tsc_khz)
+               return;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+       loops_per_jiffy = lpj;
+}
+
 /*
  * Determine if we were loaded by an EFI loader.  If so, then we have also been
  * passed the efi memmap, systab, etc., so we should use these data structures
@@ -983,6 +1003,8 @@ void __init setup_arch(char **cmdline_p)
         */
        x86_configure_nx();
 
+       simple_udelay_calibration();
+
        parse_early_param();


If it's okay for you, do you want it in a separated patch or part of patch 2/4?

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi,

On 01/26/2017 12:16 AM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote:
>
>>> What is timeout and why?
>> Put it in simple:
>>
>> The driver sets the RUN bit in control register and polls READY
>> bit in status register for the successful USB device enumeration.
>> As the USB device enumeration might fail and the READY bit will
>> never be set, the driver must have a timeout logic to avoid
>> endless loop.
>>
>> More details:
>>
>> The operational model is that driver sets up all necessary registers
>> and data structures, and then starts the debug engine by setting
>> the RUN/STOP bit in the control register.
>>
>> The debug engine then brings up itself as a ready-for-enumeration
>> USB device. The USB link between host and device starts link training
>> and then host will detect the connected device. The hub driver in
>> host will then starts the USB device enumeration processes (as defined
>> in USB spec). If everything goes smoothly, the device gets enumerated
>> and host can talk with the debug device.
>>
>> After that, xdbc firmware will set the READY bit in status register. And
>> the driver can go ahead with data transfer over USB.
> I have vague memories from a prior discussion where you said this READY
> state can be lost at any time (cable unplug or whatnot) and at that
> point the driver should re-start the setup, right?

Yes. So the documentation requires users not to unplug the usb
cable during debugging. This rule applies to other debug methods
as well.

>
>>>  If there is an error other than !ready, I would
>>> expect the hardware to inform you of this through another status bit,
>>> no?
>> Yeah, this might be another choice of hardware design. But it's not a
>> topic for this driver.
> So is there really no way to way to distinguish between "I did setup and
> am waiting for READY", "I did setup, am waiting for READY, but things
> got hosed" and "I was READY, things be hosed" ?
>
> I suppose the first and last can be distinguished by remembering if you
> ever saw READY, but the first and second are the interesting case I
> think.
>
>>> So why can't you poll indefinitely for either ready or error?
>>>
>> Even if the hardware has both ready and error status bits, it's still
>> nice to have a time out watch dog. Buggy hardware or firmware
>> might not set any of these bits. Polling indefinitely might result in
>> a endless loop.
> Loosing output, esp. without indication, is very _very_ annoying when
> you're debugging things. Its just about on par with a stuck system, at
> least then you know something bad happened.

Fair enough.

USB connection is stable enough, unless the user unplugs the
USB cable during debugging.

Best regards,
Lu Baolu

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Fair enough.
> 
> USB connection is stable enough, unless the user unplugs the
> USB cable during debugging.

What does the hardware do in this case? The XHCI registers are in the host 
hardware, so they won't disappear, right? Is there some cable connection status 
bit we can extract without interrupts?

I.e. if there's any polling component then it would be reasonable to add an error 
component: poll the status and if it goes 'disconnected' then disable early-printk 
altogether in this case and trigger an emergency printk() so that there's chance 
that the user notices [if the system does not misbehave otherwise].

I.e. try to be as robust and informative as lockdep - yet don't lock up the host 
kernel: lockdep too is called from very deep internals, there are various 
conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system 
environment outside the normal bounds of operation), yet of the kernel and over 
the last 10+ years of lockdep's existence we had very, very few cases of lockdep 
itself locking up and behaving unpredictably.

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> 
> Hi,
> 
> On 01/25/2017 10:38 PM, Peter Zijlstra wrote:
> > On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
> >> In my driver, udelay() is mostly used to handle time out.
> >>
> >> Xdbc hides most USB things in its firmware. Early printk driver only needs
> >> to setup the registers/data structures and wait until link ready or time out.
> >> Without udelay(), I have no means to convert the polling times into waiting
> >> time.
> > What is timeout and why?
> 
> Put it in simple:
> 
> The driver sets the RUN bit in control register and polls READY
> bit in status register for the successful USB device enumeration.
> As the USB device enumeration might fail and the READY bit will
> never be set, the driver must have a timeout logic to avoid
> endless loop.

Is there any error status available in the host registers anywhere that tells us 
that enumeration did not succeed?

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

On 01/26/2017 03:19 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Fair enough.
>>
>> USB connection is stable enough, unless the user unplugs the
>> USB cable during debugging.
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection status 
> bit we can extract without interrupts?

Yes, there are register bits for us to know the cable status. I will go
through the spec again and give you more accurate answer later.

I'm sorry. I will be off during the next 7 days for Chinese New Year
holiday. My email access will be very limited during this time. I will
revisit this thread after I am back from holiday.

Sorry for the inconvenience.

Best regards,
Lu Baolu

> I.e. if there's any polling component then it would be reasonable to add an error 
> component: poll the status and if it goes 'disconnected' then disable early-printk 
> altogether in this case and trigger an emergency printk() so that there's chance 
> that the user notices [if the system does not misbehave otherwise].
>
> I.e. try to be as robust and informative as lockdep - yet don't lock up the host 
> kernel: lockdep too is called from very deep internals, there are various 
> conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system 
> environment outside the normal bounds of operation), yet of the kernel and over 
> the last 10+ years of lockdep's existence we had very, very few cases of lockdep 
> itself locking up and behaving unpredictably.
>
> Thanks,
>
> 	Ingo
>

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Ingo,
> 
> On 01/26/2017 03:19 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >> Fair enough.
> >>
> >> USB connection is stable enough, unless the user unplugs the
> >> USB cable during debugging.
> > What does the hardware do in this case? The XHCI registers are in the host 
> > hardware, so they won't disappear, right? Is there some cable connection status 
> > bit we can extract without interrupts?
> 
> Yes, there are register bits for us to know the cable status. I will go
> through the spec again and give you more accurate answer later.

Ok, that's good news - so we don't really have to time out and we don't have to 
rely on the user holding the phone right either.

> I'm sorry. I will be off during the next 7 days for Chinese New Year
> holiday. My email access will be very limited during this time. I will
> revisit this thread after I am back from holiday.
> 
> Sorry for the inconvenience.

No problem, have fun!

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> > Fair enough.
> > 
> > USB connection is stable enough, unless the user unplugs the
> > USB cable during debugging.
> 
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection status 
> bit we can extract without interrupts?
> 
> I.e. if there's any polling component then it would be reasonable to add an error 
> component: poll the status and if it goes 'disconnected' then disable early-printk 
> altogether in this case and trigger an emergency printk() so that there's chance 
> that the user notices [if the system does not misbehave otherwise].

That'll be fun when printk() == early_printk() :-)

I myself wouldn't mind the system getting stuck until the link is
re-established. My own damn fault for taking that cable out etc.

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote:
> > 
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> > 
> > > Fair enough.
> > > 
> > > USB connection is stable enough, unless the user unplugs the
> > > USB cable during debugging.
> > 
> > What does the hardware do in this case? The XHCI registers are in the host 
> > hardware, so they won't disappear, right? Is there some cable connection status 
> > bit we can extract without interrupts?
> > 
> > I.e. if there's any polling component then it would be reasonable to add an error 
> > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > altogether in this case and trigger an emergency printk() so that there's chance 
> > that the user notices [if the system does not misbehave otherwise].
> 
> That'll be fun when printk() == early_printk() :-)

My suggestion would be to just print into the printk buffer directly in this case, 
without console output - the developer will notice it in 'dmesg'.

> I myself wouldn't mind the system getting stuck until the link is
> re-established. My own damn fault for taking that cable out etc.

That's fine too, although beyond the obvious "yanked the cable without realizing 
it" case there are corner cases where usability is increased massively if the 
kernel is more proactive about error conditions: for example there are 
sub-standard USB cables and there are too long USB pathways from overloaded USB 
hubs which can result in intermittent behavior, etc.

A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
about the USB-debug dongle device is a _lot_ more useful when troubleshooting such 
problems than the occasional weird, non-deterministic hang...

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > 
> > > I.e. if there's any polling component then it would be reasonable to add an error 
> > > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > > altogether in this case and trigger an emergency printk() so that there's chance 
> > > that the user notices [if the system does not misbehave otherwise].
> > 
> > That'll be fun when printk() == early_printk() :-)
> 
> My suggestion would be to just print into the printk buffer directly in this case, 
> without console output - the developer will notice it in 'dmesg'.

When you map printk() onto early_printk() dmesg will be empty, there
will be nothing there, and therefore no reason what so ever to look
there. I certainly don't ever look there.

Note that the printk buffer itself is a major part of why printk sucks
donkey balls.  Not to mention that you really cannot have an
early_printk() implementation that depends on printk().

> > I myself wouldn't mind the system getting stuck until the link is
> > re-established. My own damn fault for taking that cable out etc.
> 
> That's fine too, although beyond the obvious "yanked the cable without realizing 
> it" case there are corner cases where usability is increased massively if the 
> kernel is more proactive about error conditions: for example there are 
> sub-standard USB cables and there are too long USB pathways from overloaded USB 
> hubs which can result in intermittent behavior, etc.
> 
> A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
> about the USB-debug dongle device is a _lot_ more useful when troubleshooting such 
> problems than the occasional weird, non-deterministic hang...

Sure, I'm just not sure what or where makes sense.

If your serial cable is bad you notice because you don't receive the
right amount of characters and or stuff gets mangled. You chuck the
cable and get a new one.

I think the most important part is re-establishing the link when the
cable gets re-inserted. Maybe we should just drop all characters written
when there's no link and leave it at that, same as serial.

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > > 
> > > > I.e. if there's any polling component then it would be reasonable to add an error 
> > > > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > > > altogether in this case and trigger an emergency printk() so that there's chance 
> > > > that the user notices [if the system does not misbehave otherwise].
> > > 
> > > That'll be fun when printk() == early_printk() :-)
> > 
> > My suggestion would be to just print into the printk buffer directly in this case, 
> > without console output - the developer will notice it in 'dmesg'.
> 
> When you map printk() onto early_printk() dmesg will be empty, there
> will be nothing there, and therefore no reason what so ever to look
> there.

Unless you want a third layer of a console driver putting the debug message into 
dmesg isn't all that bad of a solution.

Let's admit it: something like USB that involves external pieces of hardware 
_does_ have failure modes, and troubleshooting messages instead of indefinite 
hangs are obviously more robust.

> I certainly don't ever look there.

You'll have to teach yourself that if the box boots up fine but there are no 
messages whatsoever from the early-printk console that you'll need to look at 
dmesg output or the syslog for more clues.

This should not be a common occurrance in any case - but when it happens it's very 
useful to have diagnostic messages. I don't think this is a controversial point in 
any fashion.

> Note that the printk buffer itself is a major part of why printk sucks donkey 
> balls.  Not to mention that you really cannot have an early_printk() 
> implementation that depends on printk().

There are several easy solutions to do that, my favorite would be to put it into 
the printk buffer totally unlocked. When your early-printk is active it's unused 
and in the end it's a known data structure after all:

/*
 * Just zap whatever's in the printk buffer and put your emergency message into 
 * it, prominently. No locking, no worries - don't generate emergency messages
 * while printk is active and syslogd is running - this facility is a poor man's 
 * fallback printk() when early-printk has taken over all kernel logging:
 */
void printk_emergency_puts(const char *str)
{
	struct printk_log *msg, *msg_end;

	msg = log_buf;

	memset(msg, 0, sizeof(*msg));	
	msg.text_len = strlen(str);

	msg_end = (void *)msg + sizeof(*msg) + msg->text_len;

	/* Zero ->len denotes end of log buffer: */
	memset(msg_end, 0, sizeof(*msg_end));	

	snprintf(ptr, str);
}

...
	printk_emergency_puts"earlyprintk emergency: Hardware timed out, shutting down. Fix your debug cable?\n");
...

(Or so - totally untested, some details might be wrong.)

But yes, I agree with your wider point, I just looked at kernel/printk/printk.c 
and puked. Why did we merge that crappy piece of binary logging code, when we 
already have two other binary logging facilities in the kernel already, both of 
them better and cleaner than this?? Why did we mess up our nicely readable, 
simple, reliable ASCII log buffer printk code? :-(

> > > I myself wouldn't mind the system getting stuck until the link is 
> > > re-established. My own damn fault for taking that cable out etc.
> > 
> > That's fine too, although beyond the obvious "yanked the cable without 
> > realizing it" case there are corner cases where usability is increased 
> > massively if the kernel is more proactive about error conditions: for example 
> > there are sub-standard USB cables and there are too long USB pathways from 
> > overloaded USB hubs which can result in intermittent behavior, etc.
> > 
> > A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
> > about the USB-debug dongle device is a _lot_ more useful when troubleshooting 
> > such problems than the occasional weird, non-deterministic hang...
> 
> Sure, I'm just not sure what or where makes sense.
> 
> If your serial cable is bad you notice because you don't receive the right 
> amount of characters and or stuff gets mangled. You chuck the cable and get a 
> new one.
> 
> I think the most important part is re-establishing the link when the cable gets 
> re-inserted. Maybe we should just drop all characters written when there's no 
> link and leave it at that, same as serial.

That would be fine with me too - but even in this case there should be a stat 
counter somewhere (in /proc or /debug) that counts the number of characters 
dropped. Maybe that file could also display an emergency string - avoiding the 
interaction with the printk buffer.

We can do better than passive-aggressive logging behavior...

Thanks,

	Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

On 01/26/2017 03:19 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Fair enough.
>>
>> USB connection is stable enough, unless the user unplugs the
>> USB cable during debugging.
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection status 
> bit we can extract without interrupts?

The hardware has a register for the debug port status. Software is able to
check cable unplugging through this register.

>
> I.e. if there's any polling component then it would be reasonable to add an error 
> component: poll the status and if it goes 'disconnected' then disable early-printk 
> altogether in this case and trigger an emergency printk() so that there's chance 
> that the user notices [if the system does not misbehave otherwise].

Sure. I will add a kernel thread to polling the errors. I will show you the
code in the new upcoming version.

Best regards,
Lu Baolu

>
> I.e. try to be as robust and informative as lockdep - yet don't lock up the host 
> kernel: lockdep too is called from very deep internals, there are various 
> conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system 
> environment outside the normal bounds of operation), yet of the kernel and over 
> the last 10+ years of lockdep's existence we had very, very few cases of lockdep 
> itself locking up and behaving unpredictably.
>
> Thanks,
>
> 	Ingo
>

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Ingo,

On 01/26/2017 03:22 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi,
>>
>> On 01/25/2017 10:38 PM, Peter Zijlstra wrote:
>>> On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
>>>> In my driver, udelay() is mostly used to handle time out.
>>>>
>>>> Xdbc hides most USB things in its firmware. Early printk driver only needs
>>>> to setup the registers/data structures and wait until link ready or time out.
>>>> Without udelay(), I have no means to convert the polling times into waiting
>>>> time.
>>> What is timeout and why?
>> Put it in simple:
>>
>> The driver sets the RUN bit in control register and polls READY
>> bit in status register for the successful USB device enumeration.
>> As the USB device enumeration might fail and the READY bit will
>> never be set, the driver must have a timeout logic to avoid
>> endless loop.
> Is there any error status available in the host registers anywhere that tells us 
> that enumeration did not succeed?

No, there isn't. The xhci spec requires software to impose a timeout.

Page 425, xhci specification:

"
Software shall impose a timeout between the detection
of the Debug Host connection and the DbC Run transition
to ‘1’.
"

Best regards,
Lu Baolu