linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
@ 2017-02-14  2:27 Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 1/5] x86: add simple udelay calibration Lu Baolu
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel, 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.

---
Change log:
v6->v7:
  - add a new patch "[PATCH 1/5] x86: add simple udelay
    calibration" to make udelay() work for early drivers.
  - [PATCH 2/5] usb: dbc: early driver for xhci debug capability
    - add a kernel thread to handle error cases, such as cable
      unplugging.
    - Fixed several code styles pointed out by Ingo.

v5->v6:
  - rebase the patches on top of the latest 4.10-rc4
  - run successfully in a 32-bit kernel
  - [PATCH 1/4]
    - remove ugly linebreaks to make code more readable
    - rename config names to make them consistency
    - move sleep-able ioremap() out of the lock area
    - rename reserved fields of register structures
    - make the vertical tabulation of struct fields consistent
  - [PATCH 2/4]
    - remove "#ifdef" in the generic code by creating proper
      wrappers in header file
  - [PATCH 3/4]
    - refine the title and commit message
  - [PATCH 4/4]
    - fix several typos and grammar errors in the document

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 (5):
  x86: add simple udelay calibration
  usb: dbc: early driver for xhci debug capability
  x86: add support for earlyprintk via USB3 debug port
  usb: serial: add dbc debug device support to usb_debug
  usb: doc: add document for USB3 debug port usage

 Documentation/admin-guide/kernel-parameters.txt |    1 +
 Documentation/usb/usb3-debug-port.rst           |   98 +++
 arch/x86/Kconfig.debug                          |   17 +
 arch/x86/kernel/early_printk.c                  |    5 +
 arch/x86/kernel/setup.c                         |   26 +
 drivers/usb/Kconfig                             |    3 +
 drivers/usb/Makefile                            |    2 +-
 drivers/usb/early/Makefile                      |    1 +
 drivers/usb/early/xhci-dbc.c                    | 1026 +++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h                    |  210 +++++
 drivers/usb/serial/usb_debug.c                  |   28 +-
 include/linux/usb/xhci-dbgp.h                   |   29 +
 12 files changed, 1442 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v7 1/5] x86: add simple udelay calibration
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
@ 2017-02-14  2:27 ` Lu Baolu
  2017-02-14  9:23   ` Sergei Shtylyov
  2017-02-14  2:27 ` [PATCH v7 2/5] usb: dbc: early driver for xhci debug capability Lu Baolu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel, Lu Baolu

Add a simple udelay calibration in x86 architecture-specific
boot-time initializations. This will get a workable estimate
for loops_per_jiffy. Hence, udelay() could be used after this
initialization.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

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();
 
 #ifdef CONFIG_MEMORY_HOTPLUG
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 2/5] usb: dbc: early driver for xhci debug capability
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 1/5] x86: add simple udelay calibration Lu Baolu
@ 2017-02-14  2:27 ` Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 3/5] x86: add support for earlyprintk via USB3 debug port Lu Baolu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel, 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 the xHCI host. There isn't any precondition
from the xHCI host side for the DbC to work.

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

Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/x86/Kconfig.debug        |   17 +
 drivers/usb/Kconfig           |    3 +
 drivers/usb/Makefile          |    2 +-
 drivers/usb/early/Makefile    |    1 +
 drivers/usb/early/xhci-dbc.c  | 1026 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h  |  210 +++++++++
 include/linux/usb/xhci-dbgp.h |   29 ++
 7 files changed, 1287 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..2216256 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -5,6 +5,9 @@ config TRACE_IRQFLAGS_SUPPORT
 
 source "lib/Kconfig.debug"
 
+config EARLY_PRINTK_USB
+	bool
+
 config X86_VERBOSE_BOOTUP
 	bool "Enable verbose x86 bootup info messages"
 	default y
@@ -29,6 +32,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
 	bool "Early printk via EHCI debug port"
 	depends on EARLY_PRINTK && PCI
+	select EARLY_PRINTK_USB
 	---help---
 	  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +52,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_USB_XDBC
+	bool "Early printk via the xHCI debug port"
+	depends on EARLY_PRINTK && PCI
+	select EARLY_PRINTK_USB
+	---help---
+	  Write kernel log output directly into the xHCI debug port.
+
+	  This is useful for kernel debugging when your machine crashes very
+	  early before the console code is initialized. For normal operation
+	  it is not recommended because it looks ugly and doesn't cooperate
+	  with klogd/syslogd or the X server. You should normally N here,
+	  unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
 	def_bool n
 
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..53d1356 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_EARLY_PRINTK_USB)	+= 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..fcde228 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_USB_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..3250172
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1026 @@
+/**
+ * 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 <linux/kthread.h>
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static bool early_console_keep;
+
+#define XDBC_TRACE
+#ifdef XDBC_TRACE
+#define	xdbc_trace	trace_printk
+#else
+static inline void xdbc_trace(const char *fmt, ...) { }
+#endif /* XDBC_TRACE */
+
+static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
+{
+	u64 val64, sz64, mask64;
+	void __iomem *base;
+	u32 val, sz;
+	u8 byte;
+
+	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	= 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	|= ~0ULL << 32;
+	}
+
+	sz64 &= mask64;
+
+	if (!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 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;
+		udelay(delay);
+		wait -= delay;
+	} while (wait > 0);
+
+	return -ETIMEDOUT;
+}
+
+static void __init xdbc_bios_handoff(void)
+{
+	int offset, timeout;
+	u32 val;
+
+	offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_LEGACY);
+	val = readl(xdbc.xhci_base + offset);
+
+	if (val & XHCI_HC_BIOS_OWNED) {
+		writel(val | XHCI_HC_OS_OWNED, xdbc.xhci_base + offset);
+		timeout = handshake(xdbc.xhci_base + offset, XHCI_HC_BIOS_OWNED, 0, 5000, 10);
+
+		if (timeout) {
+			pr_notice("xHCI BIOS handoff failed\n");
+			writel(val & ~XHCI_HC_BIOS_OWNED, xdbc.xhci_base + offset);
+		}
+	}
+
+	/* Disable any BIOS SMIs and clear all SMI events: */
+	val = readl(xdbc.xhci_base + offset + XHCI_LEGACY_CONTROL_OFFSET);
+	val &= XHCI_LEGACY_DISABLE_SMI;
+	val |= XHCI_LEGACY_SMI_EVENTS;
+	writel(val, xdbc.xhci_base + 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_segment *seg = ring->segment;
+	struct xdbc_trb *link_trb;
+
+	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_ep_context *ep_in, *ep_out;
+	struct usb_string_descriptor *s_desc;
+	struct xdbc_erst_entry *entry;
+	struct xdbc_strings *strings;
+	struct xdbc_context *ctx;
+	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->__reserved_0	= 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;
+
+	/* Popluate the 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;
+	strings		 = (struct xdbc_strings *)xdbc.string_base;
+
+	index += XDBC_STRING_ENTRY_NUM;
+
+	/* 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->manufacturer;
+	s_desc->bLength		= (strlen(XDBC_STRING_MANUFACTURER) + 1) * 2;
+	s_desc->bDescriptorType	= USB_DT_STRING;
+
+	xdbc_put_utf16(s_desc->wData, XDBC_STRING_MANUFACTURER, strlen(XDBC_STRING_MANUFACTURER));
+	string_length += s_desc->bLength;
+	string_length <<= 8;
+
+	/* String0: */
+	strings->string0[0]	= 4;
+	strings->string0[1]	= USB_DT_STRING;
+	strings->string0[2]	= 0x09;
+	strings->string0[3]	= 0x04;
+
+	string_length += 4;
+
+	/* Populate info Context: */
+	ctx = (struct xdbc_context *)xdbc.dbcc_base;
+
+	ctx->info.string0	= cpu_to_le64(xdbc.string_dma);
+	ctx->info.manufacturer	= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
+	ctx->info.product	= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
+	ctx->info.serial	= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
+	ctx->info.length	= cpu_to_le32(string_length);
+
+	/* Populate bulk out endpoint context: */
+	max_burst = DEBUG_MAX_BURST(readl(&xdbc.xdbc_reg->control));
+	ep_out = (struct xdbc_ep_context *)&ctx->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);
+
+	/* Populate bulk in endpoint context: */
+	ep_in = (struct xdbc_ep_context *)&ctx->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);
+
+	/* Set DbC context and info registers: */
+	xdbc_write64(xdbc.dbcc_dma, &xdbc.xdbc_reg->dccp);
+	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)
+{
+	void __iomem *ops_reg;
+	void __iomem *portsc;
+	u32 val, cap_length;
+	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);
+		if (!(val & PORT_CONNECT))
+			writel(val | PORT_RESET, portsc);
+	}
+}
+
+static void xdbc_reset_debug_port(void)
+{
+	u32 val, port_offset, port_count;
+	int offset = 0;
+
+	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_DBC_ENABLE | CTRL_PORT_ENABLE, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control, CTRL_DBC_ENABLE, CTRL_DBC_ENABLE, 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_CONN_STATUS, PORTSC_CONN_STATUS, 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_DBC_RUN, CTRL_DBC_RUN, 5000000, 100);
+	if (ret) {
+		xdbc_trace("waiting for device configuration timed out\n");
+		return ret;
+	}
+
+	/* Check port number: */
+	status = readl(&xdbc.xdbc_reg->status);
+	if (!DCST_DEBUG_PORT(status)) {
+		xdbc_trace("invalid root hub port number\n");
+		return -ENODEV;
+	}
+
+	xdbc.port_number = DCST_DEBUG_PORT(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_bulk_transfer(void *data, int size, bool read)
+{
+	struct xdbc_ring *ring;
+	struct xdbc_trb *trb;
+	u32 length, control;
+	u32 cycle;
+	u64 addr;
+
+	if (size > XDBC_MAX_PACKET) {
+		xdbc_trace("oops: bad parameter, size %d\n", size);
+		return -EINVAL;
+	}
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED) ||
+	    !(xdbc.flags & XDBC_FLAGS_CONFIGURED) ||
+	    (!read && (xdbc.flags & XDBC_FLAGS_OUT_STALL)) ||
+	    (read && (xdbc.flags & XDBC_FLAGS_IN_STALL))) {
+
+		xdbc_trace("oops: connection 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);
+
+	/*
+	 * Add a 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 xdbc_handle_external_reset(void)
+{
+	int ret = 0;
+
+	xdbc.flags = 0;
+	writel(0, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control, CTRL_DBC_ENABLE, 0, 100000, 10);
+	if (ret)
+		goto reset_out;
+
+	xdbc_mem_init();
+
+	mmiowb();
+
+	ret = xdbc_start();
+	if (ret < 0)
+		goto reset_out;
+
+	xdbc_trace("dbc recovered\n");
+
+	xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
+
+	xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+
+	return 0;
+
+reset_out:
+	xdbc_trace("failed to handle external reset\n");
+	return ret;
+}
+
+static int __init xdbc_early_setup(void)
+{
+	int ret;
+
+	writel(0, &xdbc.xdbc_reg->control);
+	ret = handshake(&xdbc.xdbc_reg->control, CTRL_DBC_ENABLE, 0, 100000, 100);
+	if (ret)
+		return ret;
+
+	/* Allocate the 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 the event ring: */
+	ret = xdbc_alloc_ring(&xdbc.evt_seg, &xdbc.evt_ring);
+	if (ret < 0)
+		return ret;
+
+	/* Allocate IN/OUT endpoint transfer rings: */
+	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();
+
+	mmiowb();
+
+	ret = xdbc_start();
+	if (ret < 0) {
+		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 = true;
+
+	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;
+
+	xdbc_bios_handoff();
+
+	raw_spin_lock_init(&xdbc.lock);
+
+	ret = xdbc_early_setup();
+	if (ret) {
+		pr_notice("failed to setup the connection to host\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_CONN_CHANGE) {
+		xdbc_trace("%s: connect status change event\n", __func__);
+
+		/* Check whether cable unplugged: */
+		if (!(port_reg & PORTSC_CONN_STATUS)) {
+			xdbc.flags = 0;
+			xdbc_trace("%s: cable unplugged\n", __func__);
+		}
+	}
+
+	if (port_reg & PORTSC_RESET_CHANGE)
+		xdbc_trace("%s: port reset change event\n", __func__);
+
+	if (port_reg & PORTSC_LINK_CHANGE)
+		xdbc_trace("%s: port link status change event\n", __func__);
+
+	if (port_reg & PORTSC_CONFIG_CHANGE)
+		xdbc_trace("%s: config error change\n", __func__);
+
+	/* Write back the value to clear RW1C bits: */
+	writel(port_reg, &xdbc.xdbc_reg->portsc);
+}
+
+static void xdbc_handle_tx_event(struct xdbc_trb *evt_trb)
+{
+	size_t remain_length;
+	u32 comp_code;
+	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_PACKET:
+		break;
+	case COMP_TRB_ERROR:
+	case COMP_BABBLE_DETECTED_ERROR:
+	case COMP_USB_TRANSACTION_ERROR:
+	case COMP_STALL_ERROR:
+	default:
+		if (ep_id == XDBC_EPID_OUT)
+			xdbc.flags |= XDBC_FLAGS_OUT_STALL;
+		if (ep_id == XDBC_EPID_IN)
+			xdbc.flags |= XDBC_FLAGS_IN_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;
+	u32 reg;
+	u8 cmd;
+
+	cmd = read_pci_config_byte(xdbc.bus, xdbc.dev, xdbc.func, PCI_COMMAND);
+	if (!(cmd & PCI_COMMAND_MASTER)) {
+		cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY;
+		write_pci_config_byte(xdbc.bus, xdbc.dev, xdbc.func, PCI_COMMAND, cmd);
+	}
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
+		return;
+
+	/* Handle external reset events: */
+	reg = readl(&xdbc.xdbc_reg->control);
+	if (!(reg & CTRL_DBC_ENABLE)) {
+		if (xdbc_handle_external_reset()) {
+			xdbc_trace("oops: failed to recover connection\n");
+			return;
+		}
+	}
+
+	/* Handle configure-exit event: */
+	reg = readl(&xdbc.xdbc_reg->control);
+	if (reg & CTRL_DBC_RUN_CHANGE) {
+		writel(reg, &xdbc.xdbc_reg->control);
+		if (reg & CTRL_DBC_RUN)
+			xdbc.flags |= XDBC_FLAGS_CONFIGURED;
+		else
+			xdbc.flags &= ~XDBC_FLAGS_CONFIGURED;
+	}
+
+	/* Handle endpoint stall event: */
+	reg = readl(&xdbc.xdbc_reg->control);
+	if (reg & CTRL_HALT_IN_TR) {
+		xdbc.flags |= XDBC_FLAGS_IN_STALL;
+	} else {
+		xdbc.flags &= ~XDBC_FLAGS_IN_STALL;
+		if (!(xdbc.flags & XDBC_FLAGS_IN_PROCESS))
+			xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+	}
+
+	if (reg & CTRL_HALT_OUT_TR)
+		xdbc.flags |= XDBC_FLAGS_OUT_STALL;
+	else
+		xdbc.flags &= ~XDBC_FLAGS_OUT_STALL;
+
+	/* Handle the events in the event ring: */
+	evt_trb = xdbc.evt_ring.dequeue;
+	while ((le32_to_cpu(evt_trb->field[3]) & TRB_CYCLE) == xdbc.evt_ring.cycle_state) {
+		/*
+		 * Add a memory barrier to ensure software see 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;
+		}
+
+		++(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)
+{
+	int ret, timeout = 0;
+	unsigned long flags;
+
+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 < 2000000)) {
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+		udelay(100);
+		timeout += 100;
+		goto retry;
+	}
+
+	if (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+		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)
+{
+	static char buf[XDBC_MAX_PACKET];
+	int chunk, ret;
+	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)
+				xdbc_trace("oops: missed message {%s}\n", buf);
+		}
+	}
+}
+
+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 void xdbc_unregister_console(void)
+{
+	if (early_xdbc_console.flags & CON_ENABLED)
+		unregister_console(&early_xdbc_console);
+}
+
+static int xdbc_scrub_function(void *ptr)
+{
+	unsigned long flags;
+
+	while (true) {
+		raw_spin_lock_irqsave(&xdbc.lock, flags);
+		xdbc_handle_events();
+
+		if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED)) {
+			raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+			break;
+		}
+
+		raw_spin_unlock_irqrestore(&xdbc.lock, flags);
+		schedule_timeout_interruptible(1);
+	}
+
+	xdbc_unregister_console();
+	writel(0, &xdbc.xdbc_reg->control);
+	xdbc_trace("oops: dbc scrub function exits\n");
+
+	return 0;
+}
+
+static int __init xdbc_init(void)
+{
+	unsigned long flags;
+	void __iomem *base;
+	int ret = 0;
+	u32 offset;
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
+		return 0;
+
+	/*
+	 * It's time to shut down the DbC, so that the debug
+	 * port can be reused by the host controller.
+	 */
+	if (early_xdbc_console.index == -1 ||
+	    (early_xdbc_console.flags & CON_BOOT)) {
+		xdbc_trace("hardware not used anymore\n");
+		goto free_and_quit;
+	}
+
+	base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
+	if (!base) {
+		xdbc_trace("failed to remap the io address\n");
+		ret = -ENOMEM;
+		goto free_and_quit;
+	}
+
+	raw_spin_lock_irqsave(&xdbc.lock, flags);
+	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+	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);
+
+	kthread_run(xdbc_scrub_function, NULL, "%s", "xdbc");
+
+	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..ef4a776
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.h
@@ -0,0 +1,210 @@
+/*
+ * 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	__reserved_0;	/* 0c~0f reserved bits */
+	__le64	erstba;		/* Event Ring Segment Table Base Address */
+	__le64	erdp;		/* Event Ring Dequeue Pointer */
+	__le32	control;
+	__le32	status;
+	__le32	portsc;		/* Port status and control */
+	__le32	__reserved_1;	/* 2b~28 reserved bits */
+	__le64	dccp;		/* Debug Capability Context Pointer */
+	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
+	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
+};
+
+#define DEBUG_MAX_BURST(p)	(((p) >> 16) & 0xff)
+
+#define CTRL_DBC_RUN		BIT(0)
+#define CTRL_PORT_ENABLE	BIT(1)
+#define CTRL_HALT_OUT_TR	BIT(2)
+#define CTRL_HALT_IN_TR		BIT(3)
+#define CTRL_DBC_RUN_CHANGE	BIT(4)
+#define CTRL_DBC_ENABLE		BIT(31)
+
+#define DCST_DEBUG_PORT(p)	(((p) >> 24) & 0xff)
+
+#define PORTSC_CONN_STATUS	BIT(0)
+#define PORTSC_CONN_CHANGE	BIT(17)
+#define PORTSC_RESET_CHANGE	BIT(21)
+#define PORTSC_LINK_CHANGE	BIT(22)
+#define PORTSC_CONFIG_CHANGE	BIT(23)
+
+/*
+ * xHCI Debug Capability data structures
+ */
+struct xdbc_trb {
+	__le32 field[4];
+};
+
+struct xdbc_erst_entry {
+	__le64	seg_addr;
+	__le32	seg_size;
+	__le32	__reserved_0;
+};
+
+struct xdbc_info_context {
+	__le64	string0;
+	__le64	manufacturer;
+	__le64	product;
+	__le64	serial;
+	__le32	length;
+	__le32	__reserved_0[7];
+};
+
+struct xdbc_ep_context {
+	__le32	ep_info1;
+	__le32	ep_info2;
+	__le64	deq;
+	__le32	tx_info;
+	__le32	__reserved_0[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_MANUFACTURER	"Linux"
+#define XDBC_STRING_PRODUCT		"Remote GDB"
+#define XDBC_STRING_SERIAL		"0001"
+
+struct xdbc_strings {
+	char	string0[XDBC_MAX_STRING_LENGTH];
+	char	manufacturer[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 {
+	u16			vendor;
+	u16			device;
+	u32			bus;
+	u32			dev;
+	u32			func;
+	void __iomem		*xhci_base;
+	u64			xhci_start;
+	size_t			xhci_length;
+	int			port_number;
+
+	/* DbC register base */
+	struct xdbc_regs __iomem *xdbc_reg;
+
+	/* DbC table page */
+	dma_addr_t		table_dma;
+	void			*table_base;
+
+	/* 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;
+
+	/* spinlock for early_xdbc_write() reentrancy */
+	raw_spinlock_t		lock;
+};
+#define XDBC_PCI_MAX_BUSES	256
+#define XDBC_PCI_MAX_DEVICES	32
+#define XDBC_PCI_MAX_FUNCTION	8
+
+#define XDBC_TABLE_ENTRY_SIZE	64
+#define XDBC_ERST_ENTRY_NUM	1
+#define XDBC_DBCC_ENTRY_NUM	3
+#define XDBC_STRING_ENTRY_NUM	4
+
+/* bits definitions for xdbc_state.flags */
+#define XDBC_FLAGS_INITIALIZED	BIT(0)
+#define XDBC_FLAGS_IN_STALL	BIT(1)
+#define XDBC_FLAGS_OUT_STALL	BIT(2)
+#define XDBC_FLAGS_IN_PROCESS	BIT(3)
+#define XDBC_FLAGS_OUT_PROCESS	BIT(4)
+#define XDBC_FLAGS_CONFIGURED	BIT(5)
+
+#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..80c1cca
--- /dev/null
+++ b/include/linux/usb/xhci-dbgp.h
@@ -0,0 +1,29 @@
+/*
+ * 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_USB_XDBC
+int __init early_xdbc_parse_parameter(char *s);
+int __init early_xdbc_setup_hardware(void);
+void __init early_xdbc_register_console(void);
+#else
+static inline int __init early_xdbc_setup_hardware(void)
+{
+	return -ENODEV;
+}
+static inline void __init early_xdbc_register_console(void)
+{
+}
+#endif /* CONFIG_EARLY_PRINTK_USB_XDBC */
+#endif /* __LINUX_XHCI_DBGP_H */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 3/5] x86: add support for earlyprintk via USB3 debug port
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 1/5] x86: add simple udelay calibration Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 2/5] usb: dbc: early driver for xhci debug capability Lu Baolu
@ 2017-02-14  2:27 ` Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 4/5] usb: serial: add dbc debug device support to usb_debug Lu Baolu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel, 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/admin-guide/kernel-parameters.txt | 1 +
 arch/x86/kernel/early_printk.c                  | 5 +++++
 arch/x86/kernel/setup.c                         | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index be7c0d9..4bf6138 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -988,6 +988,7 @@
 			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..0f08403 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_USB_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 aab7faa..10822b0 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -70,6 +70,7 @@
 #include <linux/tboot.h>
 #include <linux/jiffies.h>
 
+#include <linux/usb/xhci-dbgp.h>
 #include <video/edid.h>
 
 #include <asm/mtrr.h>
@@ -1142,6 +1143,9 @@ void __init setup_arch(char **cmdline_p)
 	memblock_set_current_limit(ISA_END_ADDRESS);
 	memblock_x86_fill();
 
+	if (!early_xdbc_setup_hardware())
+		early_xdbc_register_console();
+
 	reserve_bios_regions();
 
 	if (efi_enabled(EFI_MEMMAP)) {
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 4/5] usb: serial: add dbc debug device support to usb_debug
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
                   ` (2 preceding siblings ...)
  2017-02-14  2:27 ` [PATCH v7 3/5] x86: add support for earlyprintk via USB3 debug port Lu Baolu
@ 2017-02-14  2:27 ` Lu Baolu
  2017-02-14  2:27 ` [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage Lu Baolu
  2017-03-02  2:17 ` [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
  5 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel, Lu Baolu

This patch adds dbc debug device support to the 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
                   ` (3 preceding siblings ...)
  2017-02-14  2:27 ` [PATCH v7 4/5] usb: serial: add dbc debug device support to usb_debug Lu Baolu
@ 2017-02-14  2:27 ` Lu Baolu
       [not found]   ` <CAL411-o5xaA+awYi9zEZog1zCZvMCvNQ0i0R7yh24_zrTuu4gQ@mail.gmail.com>
  2017-03-02  2:17 ` [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
  5 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  2:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel,
	Lu Baolu, linux-doc

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 | 98 +++++++++++++++++++++++++++++++++++
 1 file changed, 98 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..9eddb3a
--- /dev/null
+++ b/Documentation/usb/usb3-debug-port.rst
@@ -0,0 +1,98 @@
+===============
+USB3 debug port
+===============
+
+:Author: Lu Baolu <baolu.lu@linux.intel.com>
+:Date: January 2017
+
+GENERAL
+=======
+
+This is a HOWTO for using USB3 debug port on x86 systems.
+
+Before using any kernel debugging functionality 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 purposes
+(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, "debug target" stands for
+the system under debugging, and "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 the debug target system, you need to customize a debugging
+kernel with CONFIG_EARLY_PRINTK_USB_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 starts from 0.
+
+If you are going to use 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 the debug host side, you don't need to customize the kernel,
+but you'd better 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 the 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 the debug target, DbC (xHCI debug engine for
+USB3 debug port) hardware will be detected and initialized. After
+initialization, the debug host should be able to enumerate the
+debug target as a debug device. The debug host will then bind the
+debug device with the usb_debug driver module and create the
+/dev/ttyUSB0 device.
+
+If the debug device enumeration goes smoothly, you should be able
+to see below kernel messages on the 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 the 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
       [not found]   ` <CAL411-o5xaA+awYi9zEZog1zCZvMCvNQ0i0R7yh24_zrTuu4gQ@mail.gmail.com>
@ 2017-02-14  4:41     ` Lu Baolu
  2017-02-14  6:13       ` Peter Chen
  0 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  4:41 UTC (permalink / raw)
  To: Peter Chen
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, lkml, linux-doc

Hi,

On 02/14/2017 11:45 AM, Peter Chen wrote:
> On Tue, Feb 14, 2017 at 10:27 AM, 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 | 98 ++++++++++++++++++++++++++++++
>> +++++
>>  1 file changed, 98 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..9eddb3a
>> --- /dev/null
>> +++ b/Documentation/usb/usb3-debug-port.rst
>> @@ -0,0 +1,98 @@
>> +===============
>> +USB3 debug port
>> +===============
>> +
>> +:Author: Lu Baolu <baolu.lu@linux.intel.com>
>> +:Date: January 2017
>> +
>> +GENERAL
>> +=======
>> +
>> +This is a HOWTO for using USB3 debug port on x86 systems.
>> +
>> +Before using any kernel debugging functionality 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 purposes
>> +(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, "debug target" stands for
>> +the system under debugging, and "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 the debug target system, you need to customize a debugging
>> +kernel with CONFIG_EARLY_PRINTK_USB_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 starts from 0.
>> +
>> +If you are going to use 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 the debug host side, you don't need to customize the kernel,
>> +but you'd better disable usb subsystem runtime power management
>> +by adding below kernel boot parameter::
>> +
>> +       "usbcore.autosuspend=-1"
>> +
>>
> Just curious, why autosuspend needs to be disabled for this function?

This implementation doesn't support suspend/resume yet.

Best regards,
Lu Baolu

>
> BR,
> Peter Chen
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
  2017-02-14  4:41     ` Lu Baolu
@ 2017-02-14  6:13       ` Peter Chen
  2017-02-14  7:20         ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Chen @ 2017-02-14  6:13 UTC (permalink / raw)
  To: Lu Baolu, Peter Chen
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, lkml, linux-doc

 
>
>On 02/14/2017 11:45 AM, Peter Chen wrote:
>> On Tue, Feb 14, 2017 at 10:27 AM, 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 | 98
>>> ++++++++++++++++++++++++++++++
>>> +++++
>>>  1 file changed, 98 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..9eddb3a
>>> --- /dev/null
>>> +++ b/Documentation/usb/usb3-debug-port.rst
>>> @@ -0,0 +1,98 @@
>>> +===============
>>> +USB3 debug port
>>> +===============
>>> +
>>> +:Author: Lu Baolu <baolu.lu@linux.intel.com>
>>> +:Date: January 2017
>>> +
>>> +GENERAL
>>> +=======
>>> +
>>> +This is a HOWTO for using USB3 debug port on x86 systems.
>>> +
>>> +Before using any kernel debugging functionality 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 purposes (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, "debug target" stands for the system under
>>> +debugging, and "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 the debug target system, you need to customize a debugging kernel
>>> +with CONFIG_EARLY_PRINTK_USB_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 starts
>>> +from 0.
>>> +
>>> +If you are going to use 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 the debug host side, you don't need to customize the kernel, but
>>> +you'd better disable usb subsystem runtime power management by
>>> +adding below kernel boot parameter::
>>> +
>>> +       "usbcore.autosuspend=-1"
>>> +
>>>
>> Just curious, why autosuspend needs to be disabled for this function?
>
>This implementation doesn't support suspend/resume yet.
 
Why host side needs to disable it too?

Peter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
  2017-02-14  6:13       ` Peter Chen
@ 2017-02-14  7:20         ` Lu Baolu
  0 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-14  7:20 UTC (permalink / raw)
  To: Peter Chen, Peter Chen
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, lkml, linux-doc

Hi,

On 02/14/2017 02:13 PM, Peter Chen wrote:
>  
>> On 02/14/2017 11:45 AM, Peter Chen wrote:
>>> On Tue, Feb 14, 2017 at 10:27 AM, 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 | 98
>>>> ++++++++++++++++++++++++++++++
>>>> +++++
>>>>  1 file changed, 98 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..9eddb3a
>>>> --- /dev/null
>>>> +++ b/Documentation/usb/usb3-debug-port.rst
>>>> @@ -0,0 +1,98 @@
>>>> +===============
>>>> +USB3 debug port
>>>> +===============
>>>> +
>>>> +:Author: Lu Baolu <baolu.lu@linux.intel.com>
>>>> +:Date: January 2017
>>>> +
>>>> +GENERAL
>>>> +=======
>>>> +
>>>> +This is a HOWTO for using USB3 debug port on x86 systems.
>>>> +
>>>> +Before using any kernel debugging functionality 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 purposes (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, "debug target" stands for the system under
>>>> +debugging, and "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 the debug target system, you need to customize a debugging kernel
>>>> +with CONFIG_EARLY_PRINTK_USB_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 starts
>>>> +from 0.
>>>> +
>>>> +If you are going to use 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 the debug host side, you don't need to customize the kernel, but
>>>> +you'd better disable usb subsystem runtime power management by
>>>> +adding below kernel boot parameter::
>>>> +
>>>> +       "usbcore.autosuspend=-1"
>>>> +
>>>>
>>> Just curious, why autosuspend needs to be disabled for this function?
>> This implementation doesn't support suspend/resume yet.
>  
> Why host side needs to disable it too?

If host side runtime suspend/resume is enabled, it might ask the
debug device to suspend. This will cause the debug device dead.

The suspend/resume of the debug device depends on xhci driver.
I will make it happen in separated patches with more tests. It's in
my TODO list.

Best regards,
Lu Baolu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 1/5] x86: add simple udelay calibration
  2017-02-14  2:27 ` [PATCH v7 1/5] x86: add simple udelay calibration Lu Baolu
@ 2017-02-14  9:23   ` Sergei Shtylyov
  2017-02-15  2:33     ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Sergei Shtylyov @ 2017-02-14  9:23 UTC (permalink / raw)
  To: Lu Baolu, Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel

Hello!

On 2/14/2017 5:27 AM, Lu Baolu wrote:

> Add a simple udelay calibration in x86 architecture-specific
> boot-time initializations. This will get a workable estimate
> for loops_per_jiffy. Hence, udelay() could be used after this
> initialization.
>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: x86@kernel.org
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> 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;

    Why not:

	if (!tsc_khz)
		tsc_khz = cpu_khz;

    It's more clear IMHO.

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

MBR, Sergei

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 1/5] x86: add simple udelay calibration
  2017-02-14  9:23   ` Sergei Shtylyov
@ 2017-02-15  2:33     ` Lu Baolu
  0 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-02-15  2:33 UTC (permalink / raw)
  To: Sergei Shtylyov, Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel

Hi,

On 02/14/2017 05:23 PM, Sergei Shtylyov wrote:
> Hello!
>
> On 2/14/2017 5:27 AM, Lu Baolu wrote:
>
>> Add a simple udelay calibration in x86 architecture-specific
>> boot-time initializations. This will get a workable estimate
>> for loops_per_jiffy. Hence, udelay() could be used after this
>> initialization.
>>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: x86@kernel.org
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>  arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> 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;
>
>    Why not:
>
>     if (!tsc_khz)
>         tsc_khz = cpu_khz;
>
>    It's more clear IMHO.

Sure.

Best regards,
Lu Baolu

>
>> +    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
> [...]
>
> MBR, Sergei
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
  2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
                   ` (4 preceding siblings ...)
  2017-02-14  2:27 ` [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage Lu Baolu
@ 2017-03-02  2:17 ` Lu Baolu
  2017-03-02  6:40   ` Ingo Molnar
  5 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2017-03-02  2:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ingo Molnar
  Cc: Mathias Nyman, tglx, peterz, linux-usb, x86, linux-kernel

Hi Ingo,

How about this version? Any further comments?

Best regards,
Lu  Baolu

On 02/14/2017 10:27 AM, 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.
>
> ---
> Change log:
> v6->v7:
>   - add a new patch "[PATCH 1/5] x86: add simple udelay
>     calibration" to make udelay() work for early drivers.
>   - [PATCH 2/5] usb: dbc: early driver for xhci debug capability
>     - add a kernel thread to handle error cases, such as cable
>       unplugging.
>     - Fixed several code styles pointed out by Ingo.
>
> v5->v6:
>   - rebase the patches on top of the latest 4.10-rc4
>   - run successfully in a 32-bit kernel
>   - [PATCH 1/4]
>     - remove ugly linebreaks to make code more readable
>     - rename config names to make them consistency
>     - move sleep-able ioremap() out of the lock area
>     - rename reserved fields of register structures
>     - make the vertical tabulation of struct fields consistent
>   - [PATCH 2/4]
>     - remove "#ifdef" in the generic code by creating proper
>       wrappers in header file
>   - [PATCH 3/4]
>     - refine the title and commit message
>   - [PATCH 4/4]
>     - fix several typos and grammar errors in the document
>
> 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 (5):
>   x86: add simple udelay calibration
>   usb: dbc: early driver for xhci debug capability
>   x86: add support for earlyprintk via USB3 debug port
>   usb: serial: add dbc debug device support to usb_debug
>   usb: doc: add document for USB3 debug port usage
>
>  Documentation/admin-guide/kernel-parameters.txt |    1 +
>  Documentation/usb/usb3-debug-port.rst           |   98 +++
>  arch/x86/Kconfig.debug                          |   17 +
>  arch/x86/kernel/early_printk.c                  |    5 +
>  arch/x86/kernel/setup.c                         |   26 +
>  drivers/usb/Kconfig                             |    3 +
>  drivers/usb/Makefile                            |    2 +-
>  drivers/usb/early/Makefile                      |    1 +
>  drivers/usb/early/xhci-dbc.c                    | 1026 +++++++++++++++++++++++
>  drivers/usb/early/xhci-dbc.h                    |  210 +++++
>  drivers/usb/serial/usb_debug.c                  |   28 +-
>  include/linux/usb/xhci-dbgp.h                   |   29 +
>  12 files changed, 1442 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
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
  2017-03-02  2:17 ` [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
@ 2017-03-02  6:40   ` Ingo Molnar
  2017-03-03  8:22     ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Ingo Molnar @ 2017-03-02  6:40 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, linux-kernel


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

> Hi Ingo,
> 
> How about this version? Any further comments?

So I have re-read the review feedback I gave on Jan 19 and found at least one 
thing I pointed out that you didn't address in the latest patches ...

Plus please go beyond the feedback given - for example the Kconfig text contains 
at least one typo.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
  2017-03-02  6:40   ` Ingo Molnar
@ 2017-03-03  8:22     ` Lu Baolu
  2017-03-16  7:17       ` Ingo Molnar
  0 siblings, 1 reply; 16+ messages in thread
From: Lu Baolu @ 2017-03-03  8:22 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, linux-kernel

Hi Ingo,

On 03/02/2017 02:40 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Ingo,
>>
>> How about this version? Any further comments?
> So I have re-read the review feedback I gave on Jan 19 and found at least one 
> thing I pointed out that you didn't address in the latest patches ...

Do you mind telling me which one is not addressed? Is it one of below
feedbacks?

==============================
>
> 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.
-----------------------------------------------------------------
>> 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’.
"
-----------------------------------------------------------------
> +
> +    val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
> +    sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> +    mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;

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

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

I posted all feedbacks I received by now at the tail of this email to
make sure all feedbacks were communicated and addressed.


>
> Plus please go beyond the feedback given - for example the Kconfig text contains 
> at least one typo.
>
>

I am sorry about this.

I will try my best to make typos and code style clean.

Best regards,
Lu Baolu

[Below are all feedbacks.]


=================================================================
[PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
=================================================================
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.
-----------------------------------------------------------------
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?
-----------------------------------------------------------------
> +    if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +        PCI_BASE_ADDRESS_MEM_TYPE_64) {

Please don't break the line here.
-----------------------------------------------------------------
> +        val64 |= ((u64)val << 32);
> +        sz64 |= ((u64)sz << 32);
> +        mask64 |= ((u64)~0 << 32);

Unnecessary parentheses.
-----------------------------------------------------------------
> +    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?
-----------------------------------------------------------------
> +    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_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.
-----------------------------------------------------------------
> +    /*
> +     * 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

?
-----------------------------------------------------------------
> +    /* 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.
-----------------------------------------------------------------
>
> 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.
-----------------------------------------------------------------
>> 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’.
"

-----------------------------------------------------------------
> between the debug host and target. The DbC functionality is
> independent of xHCI host. There isn't any precondition from
> xHCI host side for DbC to work.

s/xHCI host/the xHCI host

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

s/out/output
s/in/input

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

s/xHCI/the xHCI

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

-----------------------------------------------------------------
> +
> +    val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
> +    sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> +    mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;

Is this cast necessary?

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

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

-----------------------------------------------------------------

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

Type cast could be avoided by using 0L.

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

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

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

Please don't break this line. You can shorten the variable name if you want to
save line length.
-----------------------------------------------------------------
> +    /* populate the contexts */
> +    context = (struct xdbc_context *)xdbc.dbcc_base;
> +    context->info.string0 = cpu_to_le64(xdbc.string_dma);
> +    context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
> +    context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
> +    context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
> +    context->info.length = cpu_to_le32(string_length);

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

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

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

?
-----------------------------------------------------------------
> +    /* Check completion of the previous request. */

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

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

    /* Check completion of the previous request: */

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

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

I was thinking of something like:

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

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

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

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

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

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

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

?

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

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

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

> +#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.
-----------------------------------------------------------------

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

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

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

-----------------------------------------------------------------
s/debugging functionalities
 /debugging functionality
-----------------------------------------------------------------
s/debugging purpose
 /debugging purposes
-----------------------------------------------------------------
s/On debug target system
  On the debug target system

[End of feedback list.]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
  2017-03-03  8:22     ` Lu Baolu
@ 2017-03-16  7:17       ` Ingo Molnar
  2017-03-17  2:37         ` Lu Baolu
  0 siblings, 1 reply; 16+ messages in thread
From: Ingo Molnar @ 2017-03-16  7:17 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, linux-kernel


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

> Hi Ingo,
> 
> On 03/02/2017 02:40 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >> Hi Ingo,
> >>
> >> How about this version? Any further comments?
> > So I have re-read the review feedback I gave on Jan 19 and found at least one 
> > thing I pointed out that you didn't address in the latest patches ...
> 
> Do you mind telling me which one is not addressed? Is it one of below
> feedbacks?

So one piece of feedback I gave was:

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

But the latest Kconfig help text still says this:

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

... while in reality it's an alternative lockless logging facility that goes way 
beyond debugging early boot crashes!

Granted, I qualified that with 'just a side note'. I guess something like this 
would work:

+         One use for this feature is kernel debugging, for example when your 
+	  machine crashes very early before the regular console code is 
+	  initialized. Other uses include simpler, lockless logging instead of a 
+	  full-blown printk console driver + klogd.
+
+	  For normal production environments this is normally not recommended,
+	  because it doesn't feed events into klogd/syslogd and doesn't try to
+	  print anything on the screen.
+
+	  You should normally N here, unless you want to debug early crashes or 
+	  need a very simple printk logging facility.

Another piece of feedback I gave was:

> > +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?

yet your latest submission still includes the very same postfixed config switch 
name that collides with the prefixed names such as EARLY_PRINTK_USB:

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

The problem I tried to point out with my review feedback is that we thus have 
both:

	CONFIG_EARLY_PRINTK_USB=y
	CONFIG_USB_EARLY_PRINTK=y

... which is confusing at the very least.

On a second look, this config switch appears to be unused - is it a leftover from 
earlier patches?

The patches don't look too bad otherwise, so we are not far from having something 
acceptable, IMHO.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port
  2017-03-16  7:17       ` Ingo Molnar
@ 2017-03-17  2:37         ` Lu Baolu
  0 siblings, 0 replies; 16+ messages in thread
From: Lu Baolu @ 2017-03-17  2:37 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Greg Kroah-Hartman, Ingo Molnar, Mathias Nyman, tglx, peterz,
	linux-usb, x86, linux-kernel

Hi Ingo,

On 03/16/2017 03:17 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Ingo,
>>
>> On 03/02/2017 02:40 PM, Ingo Molnar wrote:
>>> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>
>>>> Hi Ingo,
>>>>
>>>> How about this version? Any further comments?
>>> So I have re-read the review feedback I gave on Jan 19 and found at least one 
>>> thing I pointed out that you didn't address in the latest patches ...
>> Do you mind telling me which one is not addressed? Is it one of below
>> feedbacks?
> So one piece of feedback I gave was:
>
> | 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.
>
> But the latest Kconfig help text still says this:
>
> +config EARLY_PRINTK_USB_XDBC
> +       bool "Early printk via the xHCI debug port"
> +       depends on EARLY_PRINTK && PCI
> +       select EARLY_PRINTK_USB
> +       ---help---
> +         Write kernel log output directly into the xHCI debug port.
> +
> +         This is useful for kernel debugging when your machine crashes very
> +         early before the console code is initialized. For normal operation
> +         it is not recommended because it looks ugly and doesn't cooperate
> +         with klogd/syslogd or the X server. You should normally N here,
> +         unless you want to debug such a crash.
>
> ... while in reality it's an alternative lockless logging facility that goes way 
> beyond debugging early boot crashes!
>
> Granted, I qualified that with 'just a side note'. I guess something like this 
> would work:
>
> +         One use for this feature is kernel debugging, for example when your 
> +	  machine crashes very early before the regular console code is 
> +	  initialized. Other uses include simpler, lockless logging instead of a 
> +	  full-blown printk console driver + klogd.
> +
> +	  For normal production environments this is normally not recommended,
> +	  because it doesn't feed events into klogd/syslogd and doesn't try to
> +	  print anything on the screen.
> +
> +	  You should normally N here, unless you want to debug early crashes or 
> +	  need a very simple printk logging facility.

Very appreciated for pointing this out. I will replace it.

>
> Another piece of feedback I gave was:
>
>>> +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?
> yet your latest submission still includes the very same postfixed config switch 
> name that collides with the prefixed names such as EARLY_PRINTK_USB:
>
> --- 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
>
> The problem I tried to point out with my review feedback is that we thus have 
> both:
>
> 	CONFIG_EARLY_PRINTK_USB=y
> 	CONFIG_USB_EARLY_PRINTK=y
>
> ... which is confusing at the very least.
>
> On a second look, this config switch appears to be unused - is it a leftover from 
> earlier patches?

Yes, it is a leftover from the earlier patches. I should remove it.
Sorry about it.

>
> The patches don't look too bad otherwise, so we are not far from having something 
> acceptable, IMHO.
>

Thanks.

For the typo and grammar issues, I will recheck the patches and ask some
English speakers for review.

Best regards,
Lu Baolu

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-03-17  2:37 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-14  2:27 [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
2017-02-14  2:27 ` [PATCH v7 1/5] x86: add simple udelay calibration Lu Baolu
2017-02-14  9:23   ` Sergei Shtylyov
2017-02-15  2:33     ` Lu Baolu
2017-02-14  2:27 ` [PATCH v7 2/5] usb: dbc: early driver for xhci debug capability Lu Baolu
2017-02-14  2:27 ` [PATCH v7 3/5] x86: add support for earlyprintk via USB3 debug port Lu Baolu
2017-02-14  2:27 ` [PATCH v7 4/5] usb: serial: add dbc debug device support to usb_debug Lu Baolu
2017-02-14  2:27 ` [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage Lu Baolu
     [not found]   ` <CAL411-o5xaA+awYi9zEZog1zCZvMCvNQ0i0R7yh24_zrTuu4gQ@mail.gmail.com>
2017-02-14  4:41     ` Lu Baolu
2017-02-14  6:13       ` Peter Chen
2017-02-14  7:20         ` Lu Baolu
2017-03-02  2:17 ` [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu
2017-03-02  6:40   ` Ingo Molnar
2017-03-03  8:22     ` Lu Baolu
2017-03-16  7:17       ` Ingo Molnar
2017-03-17  2:37         ` Lu Baolu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).