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

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

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

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

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

Cc Mathias Nyman <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 arch/x86/Kconfig.debug        |   14 +
 drivers/usb/Kconfig           |    3 +
 drivers/usb/Makefile          |    2 +-
 drivers/usb/early/Makefile    |    1 +
 drivers/usb/early/xhci-dbc.c  | 1068 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h  |  205 ++++++++
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1314 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

arch/x86/Kconfig.debug +3 -0

index 67eec55..13e85b7 100644
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
 	bool "Early printk via EHCI debug port"
 	depends on EARLY_PRINTK && PCI
+	select USB_EARLY_PRINTK
 	---help---
 	  Write kernel log output directly into the EHCI debug port.
 

@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
 	  This is useful for kernel debugging when your machine crashes very
 	  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+	bool "Early printk via xHCI debug port"

arch/x86/Kconfig.debug +10 -1

-obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK)	+= early/
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
 
+static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
+{
+	u32 val, sz;
+	u64 val64, sz64, mask64;
+	u8 byte;
+	void __iomem *base;
+
+	val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);

arch/x86/Kconfig.debug +12 -0

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

arch/x86/Kconfig.debug +5 -0

+static int __init xdbc_init(void)
+{
+	unsigned long flags;
+	void __iomem *base;
+	u32 offset;

arch/x86/Kconfig.debug +74 -0

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

Quoted 58 lines · Lu Baolu

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

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

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

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

Cc Mathias Nyman <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 arch/x86/Kconfig.debug        |   14 +
 drivers/usb/Kconfig           |    3 +
 drivers/usb/Makefile          |    2 +-
 drivers/usb/early/Makefile    |    1 +
 drivers/usb/early/xhci-dbc.c  | 1068 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/early/xhci-dbc.h  |  205 ++++++++
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1314 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

arch/x86/Kconfig.debug +3 -0

index 67eec55..13e85b7 100644
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
 	bool "Early printk via EHCI debug port"
 	depends on EARLY_PRINTK && PCI
+	select USB_EARLY_PRINTK
 	---help---
 	  Write kernel log output directly into the EHCI debug port.
 

@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
 	  This is useful for kernel debugging when your machine crashes very
 	  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+	bool "Early printk via xHCI debug port"

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

Quoted 10 lines

arch/x86/Kconfig.debug +10 -0

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

Could we please do this rename:

s/EARLY_PRINTK_XDBC
EARLY_PRINTK_USB_XDBC

?

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

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

Quoted 2 lines

arch/x86/Kconfig.debug +2 -0

+config USB_EARLY_PRINTK
+	bool

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

You can see the prefix/postfix inconsistency here already:

Quoted 14 lines

arch/x86/Kconfig.debug +12 -1

-obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK)	+= early/
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
+static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
+{
+	u32 val, sz;
+	u64 val64, sz64, mask64;
+	u8 byte;
+	void __iomem *base;
+
+	val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
+	write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);
+	sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);

arch/x86/Kconfig.debug

Quoted 7 lines

arch/x86/Kconfig.debug +7 -0

+	write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
+	if (val == 0xffffffff || sz == 0xffffffff) {
+		pr_notice("invalid mmio bar\n");
+		return NULL;
+	}
+	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+	    PCI_BASE_ADDRESS_MEM_TYPE_64) {

Please don't break the line here.

arch/x86/Kconfig.debug

Quoted 9 lines

arch/x86/Kconfig.debug +8 -0

+		val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
+		write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
+		sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
+		write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
+
+		val64 |= ((u64)val << 32);
+		sz64 |= ((u64)sz << 32);
+		mask64 |= ((u64)~0 << 32);

Unnecessary parentheses.

arch/x86/Kconfig.debug

Quoted 9 lines

arch/x86/Kconfig.debug +8 -0

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

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

arch/x86/Kconfig.debug

Quoted 10 lines

arch/x86/Kconfig.debug +9 -0

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

Please no ugly linebreaks.

arch/x86/Kconfig.debug

Quoted 6 lines

arch/x86/Kconfig.debug +5 -0

+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

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

arch/x86/Kconfig.debug

Quoted 3 lines

arch/x86/Kconfig.debug +2 -0

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

arch/x86/Kconfig.debug

Quoted 3 lines

arch/x86/Kconfig.debug +2 -0

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

arch/x86/Kconfig.debug

Quoted 4 lines

arch/x86/Kconfig.debug +4 -0

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

Quoted 17 lines

arch/x86/Kconfig.debug +16 -0

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

arch/x86/Kconfig.debug

Quoted 6 lines

arch/x86/Kconfig.debug +6 -0

+				n++;
+				continue;
+			}
+			if (use_cr)
+				use_cr = 0;
+			buf[chunk] = *str;

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

Quoted 6 lines

arch/x86/Kconfig.debug +5 -0

+static int __init xdbc_init(void)
+{
+	unsigned long flags;
+	void __iomem *base;
+	u32 offset;

arch/x86/Kconfig.debug

Quoted 10 lines

arch/x86/Kconfig.debug +10 -0

+	int ret = 0;
+
+	if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
+		return 0;
+
+	xdbc_delay = xdbc_runtime_delay;
+
+	/*
+	 * It's time to shutdown DbC, so that the debug
+	 * port could be reused by the host controller.

s/shutdown DbC
/shut down the DbC

s/could be reused
/can be reused

?

arch/x86/Kconfig.debug

Quoted 4 lines

arch/x86/Kconfig.debug +4 -0

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

s/any more
anymore

arch/x86/Kconfig.debug

Quoted 3 lines

arch/x86/Kconfig.debug +2 -0

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

Ugh, ioremap() can sleep ...

arch/x86/Kconfig.debug

Quoted 9 lines

arch/x86/Kconfig.debug +8 -0

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

arch/x86/Kconfig.debug

Quoted 4 lines

arch/x86/Kconfig.debug +3 -0

+	__le32	rsvd;
+	__le32	rsvdz[7];
+	__le32	rsvd0[11];

ditto.

arch/x86/Kconfig.debug

Quoted 8 lines

arch/x86/Kconfig.debug +7 -0

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

arch/x86/Kconfig.debug

Quoted 5 lines

arch/x86/Kconfig.debug +4 -0

+	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

?

Quoted 75 lines

arch/x86/Kconfig.debug +74 -0

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

arch/x86/Kconfig.debug

Quoted 10 lines

arch/x86/Kconfig.debug +10 -0

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

Quoted 8 lines

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

Quoted 6 lines

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

+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

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

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

Quoted 7 lines · Lu Baolu

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

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

Quoted 2 lines

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

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

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

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

?

Hi Ingo,

Quoted 9 lines · Ingo Molnar

Quoted 7 lines · Lu Baolu

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

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

Quoted 4 lines

Quoted 2 lines

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

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

Yes.

Quoted 6 lines

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

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

?

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

__const_udelay().

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

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

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

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

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

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

Quoted 55 lines · Lu Baolu

Hi Ingo,

Quoted 9 lines · Ingo Molnar

Quoted 7 lines · Lu Baolu

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+static void xdbc_runtime_delay(unsigned long count)
+{
+	udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

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

Quoted 4 lines

Quoted 2 lines

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

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

Yes.

Quoted 5 lines

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

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

?

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

__const_udelay().

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

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

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

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

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

Quoted 2 lines

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

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

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

Quoted 2 lines

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

Sure. How about below change?

drivers/usb/early/xhci-dbc.c +29 -0

index d3f0c84..940989e 100644
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
        return size;
 }
 
+static void __init xdbc_udelay_calibration(void)
+{
+       unsigned long lpj = 0;
+       unsigned int tsc_khz, cpu_khz;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               goto calibration_out;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       if (tsc_khz == 0)
+               tsc_khz = cpu_khz;
+       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+               cpu_khz = tsc_khz;
+
+       if (!tsc_khz)
+               goto calibration_out;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+
+calibration_out:
+       if (!lpj)
+               lpj = 1 << 22;
+
+       loops_per_jiffy = lpj;
+}
+
 static int __init xdbc_early_setup(void)
 {
        int ret;

Quoted 2 lines · Lu Baolu

Quoted 2 lines

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

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

Quoted 6 lines · Ingo Molnar

Quoted 2 lines · Lu Baolu

Quoted 2 lines

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

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

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

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

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

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

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

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

Quoted 6 lines · Lu Baolu

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

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

What is timeout and why?

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

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

Quoted 1 line

What is timeout and why?

Put it in simple:

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

More details:

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

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

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

Quoted 3 lines

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

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

Quoted 2 lines

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

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

Quoted 22 lines · Lu Baolu

Quoted 1 line

What is timeout and why?

Put it in simple:

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

More details:

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

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

Quoted 2 lines

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

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

Quoted 6 lines

Quoted 4 lines

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

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

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

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

Quoted 2 lines

Quoted 2 lines

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

Quoted 4 lines

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

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

Fair enough.

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

Quoted 4 lines · Lu Baolu

Fair enough.

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

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

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

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

Thanks,

Ingo

Hi Ingo,

Quoted 4 lines · Ingo Molnar

Quoted 2 lines · Lu Baolu

Fair enough.

Quoted 5 lines

Quoted 2 lines

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

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

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

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

Sorry for the inconvenience.

Quoted 4 lines · Lu Baolu

Fair enough.

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

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

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

Quoted 12 lines · Ingo Molnar

Quoted 4 lines · Lu Baolu

Fair enough.

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

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

Quoted 4 lines

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

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

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

Quoted 7 lines

Quoted 5 lines

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

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

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

Quoted 2 lines

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

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

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

Quoted 6 lines · Ingo Molnar

Quoted 6 lines

Quoted 5 lines

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

Quoted 4 lines

Quoted 1 line

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

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

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

I certainly don't ever look there.

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

Quoted 9 lines

Quoted 2 lines

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

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

Quoted 3 lines

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

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

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

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

Quoted 4 lines · Lu Baolu

Fair enough.

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

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

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

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

Thanks,

Ingo

Hi,

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Lu Baolu

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

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

What is timeout and why?

Put it in simple:

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

Quoted 13 lines · Lu Baolu

Hi,

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Lu Baolu

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

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

What is timeout and why?

Put it in simple:

Quoted 5 lines

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?

Quoted 48 lines · Lu Baolu

Quoted 2 lines

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

Sure. How about below change?

drivers/usb/early/xhci-dbc.c +29 -0

index d3f0c84..940989e 100644
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
        return size;
 }
 
+static void __init xdbc_udelay_calibration(void)
+{
+       unsigned long lpj = 0;
+       unsigned int tsc_khz, cpu_khz;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               goto calibration_out;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       if (tsc_khz == 0)
+               tsc_khz = cpu_khz;
+       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+               cpu_khz = tsc_khz;
+
+       if (!tsc_khz)
+               goto calibration_out;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+
+calibration_out:
+       if (!lpj)
+               lpj = 1 << 22;
+
+       loops_per_jiffy = lpj;
+}
+
 static int __init xdbc_early_setup(void)
 {
        int ret;

@@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
        }
        xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 

drivers/usb/early/xhci-dbc.c int __init early_xdbc_parse_parameter(char *s) · line 721

Quoted 4 lines

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

+       xdbc_udelay_calibration();
+
        return 0;
 }

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

Quoted 48 lines · Lu Baolu

Quoted 2 lines

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

Sure. How about below change?

drivers/usb/early/xhci-dbc.c +29 -0

index d3f0c84..940989e 100644
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
        return size;
 }
 
+static void __init xdbc_udelay_calibration(void)
+{
+       unsigned long lpj = 0;
+       unsigned int tsc_khz, cpu_khz;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               goto calibration_out;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       if (tsc_khz == 0)
+               tsc_khz = cpu_khz;
+       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+               cpu_khz = tsc_khz;
+
+       if (!tsc_khz)
+               goto calibration_out;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+
+calibration_out:
+       if (!lpj)
+               lpj = 1 << 22;
+
+       loops_per_jiffy = lpj;
+}
+
 static int __init xdbc_early_setup(void)
 {
        int ret;

@@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
        }
        xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 

drivers/usb/early/xhci-dbc.c int __init early_xdbc_parse_parameter(char *s) · line 721

Quoted 4 lines

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

+       xdbc_udelay_calibration();
+
        return 0;
 }

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

index 8a12199..c4031b9 100644

arch/x86/kernel/early_printk.c +5 -0

@@ -17,6 +17,7 @@
 #include <asm/intel-mid.h>
 #include <asm/pgtable.h>
 #include <linux/usb/ehci_def.h>
+#include <linux/usb/xhci-dbgp.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
 #include <asm/pci_x86.h>

@@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)
 		if (!strncmp(buf, "efi", 3))
 			early_console_register(&early_efi_console, keep);
 #endif
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+		if (!strncmp(buf, "xdbc", 4))
+			early_xdbc_parse_parameter(buf + 4);
+#endif
 
 		buf++;
 	}

arch/x86/kernel/setup.c +7 -0

index 9c337b0..09d4a56 100644
@@ -70,6 +70,8 @@
 #include <linux/tboot.h>
 #include <linux/jiffies.h>
 
+#include <linux/usb/xhci-dbgp.h>
+
 #include <video/edid.h>
 
 #include <asm/mtrr.h>

@@ -1096,6 +1098,11 @@ void __init setup_arch(char **cmdline_p)
 	memblock_set_current_limit(ISA_END_ADDRESS);
 	memblock_x86_fill();
 
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+	if (!early_xdbc_setup_hardware())
+		early_xdbc_register_console();
+#endif
+
 	reserve_bios_regions();
 
 	if (efi_enabled(EFI_MEMMAP)) {

Signature · 1 line

2.1.4

Quoted 46 lines · Lu Baolu

index 8a12199..c4031b9 100644

arch/x86/kernel/early_printk.c +5 -0

@@ -17,6 +17,7 @@
 #include <asm/intel-mid.h>
 #include <asm/pgtable.h>
 #include <linux/usb/ehci_def.h>
+#include <linux/usb/xhci-dbgp.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
 #include <asm/pci_x86.h>

@@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)
 		if (!strncmp(buf, "efi", 3))
 			early_console_register(&early_efi_console, keep);
 #endif
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+		if (!strncmp(buf, "xdbc", 4))
+			early_xdbc_parse_parameter(buf + 4);
+#endif
 
 		buf++;
 	}

arch/x86/kernel/setup.c +7 -0

index 9c337b0..09d4a56 100644
@@ -70,6 +70,8 @@
 #include <linux/tboot.h>
 #include <linux/jiffies.h>
 
+#include <linux/usb/xhci-dbgp.h>
+
 #include <video/edid.h>
 
 #include <asm/mtrr.h>

@@ -1096,6 +1098,11 @@ void __init setup_arch(char **cmdline_p)
 	memblock_set_current_limit(ISA_END_ADDRESS);
 	memblock_x86_fill();
 
+#ifdef CONFIG_EARLY_PRINTK_XDBC
+	if (!early_xdbc_setup_hardware())
+		early_xdbc_register_console();
+#endif
+
 	reserve_bios_regions();
 

Quoted 3 lines

arch/x86/kernel/setup.c +0 -0

 	if (efi_enabled(EFI_MEMMAP)) {

Signature · 1 line

2.1.4

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

This patch add dbc debug device support in usb_debug driver.

Quoted 1 line · Lu Baolu

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.

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

Cc [email protected]

Signed-off-by Lu Baolu <[email protected]>

---
 Documentation/usb/usb3-debug-port.rst | 95 +++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/usb/usb3-debug-port.rst

Documentation/usb/usb3-debug-port.rst new file +3 -0

new file mode 100644
index 0000000..70eabe4
@@ -0,0 +1,95 @@
+===============
+USB3 debug port
+===============

Quoted 19 lines · Lu Baolu

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

Cc [email protected]

Signed-off-by Lu Baolu <[email protected]>

---
 Documentation/usb/usb3-debug-port.rst | 95 +++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/usb/usb3-debug-port.rst

Documentation/usb/usb3-debug-port.rst new file +3 -0

new file mode 100644
index 0000000..70eabe4
@@ -0,0 +1,95 @@
+===============
+USB3 debug port
+===============

Documentation/usb/usb3-debug-port.rst line 13

Quoted 10 lines

Documentation/usb/usb3-debug-port.rst new file +10 -0

+
+:Author: Lu Baolu <[email protected]>
+:Date: October 2016
+
+GENERAL
+=======
+
+This is a HOWTO for using USB3 debug port on x86 systems.
+
+Before using any kernel debugging functionalities based on USB3

s/debugging functionalities
/debugging functionality

Documentation/usb/usb3-debug-port.rst line 15

Quoted 2 lines

Documentation/usb/usb3-debug-port.rst new file +2 -0

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

s/debugging purpose
/debugging purposes

Documentation/usb/usb3-debug-port.rst line 26

Quoted 1 line

Documentation/usb/usb3-debug-port.rst new file +1 -0

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

s/On debug target system
On the debug target system

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

xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. With DbC hardware initialized, the system will present a debug device through the USB3 debug port (normally the first USB3 port). The debug device is fully compliant with the USB framework and provides the equivalent of a very high performance (USB3) full-duplex serial link between the debug host and target. The DbC functionality is independent of xHCI host. There isn't any precondition from xHCI host side for DbC to work.

This patch set adds support for early printk functionality through a USB3 debug port by 1) initializing and enabling the DbC hardware during early boot; 2) registering a boot console to the system so that early printk messages can go through the USB3 debug port. It also includes some lines of changes in usb_debug driver so that it can be bound when a USB3 debug device is enumerated.

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

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

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

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

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

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

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

Hi Greg,

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

Quoted 4 lines · Lu Baolu

Hi Greg,

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

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

Quoted 3 lines · Greg Kroah-Hartman

Quoted 2 lines · Lu Baolu

Hi Greg,

Quoted 5 lines

Quoted 2 lines

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

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

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

xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. With DbC hardware initialized, the system will present a debug device through the USB3 debug port (normally the first USB3 port). The debug device is fully compliant with the USB framework and provides the equivalent of a very high performance (USB3) full-duplex serial link between the debug host and target. The DbC functionality is independent of xHCI host. There isn't any precondition from xHCI host side for DbC to work.

This patch set adds support for early printk functionality through a USB3 debug port by 1) initializing and enabling the DbC hardware during early boot; 2) registering a boot console to the system so that early printk messages can go through the USB3 debug port. It also includes some lines of changes in usb_debug driver so that it can be bound when a USB3 debug device is enumerated.

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

Quoted 18 lines · 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.

Quoted 5 lines

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

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

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