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

[Lu Baolu]

Hi,

On 09/20/2016 09:54 PM, Mathias Nyman wrote:
> On 29.08.2016 08:26, Lu Baolu wrote:
>> xHCI debug capability (DbC) is an optional but standalone
>> functionality provided by an xHCI host controller. Software
>> learns this capability by walking through the extended
>> capability list of the host. xHCI specification describes
>> DbC in section 7.6.
>>
>> This patch introduces the code to probe and initialize the
>> debug capability hardware during early boot. With hardware
>> initialized, the debug target (system on which this code is
>> running) will present a debug device through the debug port
>> (normally the first USB3 port). The debug device is fully
>> compliant with the USB framework and provides the equivalent
>> of a very high performance (USB3) full-duplex serial link
>> between the debug host and target. The DbC functionality is
>> independent of xHCI host. There isn't any precondition from
>> xHCI host side for DbC to work.
>>
>> This patch also includes bulk out and bulk in interfaces.
>> These interfaces could be used to implement early printk
>> bootconsole or hook to various system debuggers.
>>
>> This code is designed to be only used for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. For normal operation it is not recommended.
>>
>> Cc: Mathias Nyman 
>> Signed-off-by: Lu Baolu 
>> ---
>
> Some comments inline

Thank you for reviewing my patch.

>
>> +
>> +#ifdef XDBC_TRACE
>> +#definexdbc_tracetrace_printk
>> +#else
>> +static inline void xdbc_trace(const char *fmt, ...) { }
>> +#endif /* XDBC_TRACE */
>
> I guess there's probably no better way to enable/disable debug messages for 
> this
> driver this early, and ehci-dbgp does the same.
>
> So I guess It's ok, but it still looks weird
>
>> +
>> +static void xdbc_early_delay(unsigned long count)
>> +{
>> +u8 val;
>> +
>> +val = inb(0x80);
>> +while (count-- > 0)
>> +outb(val, 0x80);
>> +}
>> +
>> +static void xdbc_runtime_delay(unsigned long count)
>> +{
>> +udelay(count);
>> +}
>> +
>
> Glad to see the addition of this runtime_delay in addition
> to the hack of reading port 0x80 for a 1us delay.
>
> And that the early_delay is only used for as long as it must.
>
>
>> +static int xdbc_handle_external_reset(void)
>> +{
>> +u32 ctrl;
>> +int ret;
>> +
>> +writel(0, _reg->control);
>> +ret = handshake(_reg->control, CTRL_DCE, 0, 10, 10);
>> +if (ret)
>> +return ret;
>> +
>> +xdbc_mem_init();
>> +mmiowb();
>> +
>> +ctrl = readl(_reg->control);
>> +writel(ctrl | CTRL_DCE | CTRL_PED, _reg->control);
>> +ret = handshake(_reg->control,
>> +CTRL_DCE, CTRL_DCE, 10, 10);
>> +if (ret)
>> +return ret;
>> +
>> +if (xdbc.vendor == PCI_VENDOR_ID_INTEL)
>> +xdbc_do_reset_debug_port(xdbc.port_number, 1);
>> +
>> +/* wait for port connection */
>> +ret = handshake(_reg->portsc,
>> +PORTSC_CCS, PORTSC_CCS, 500, 10);
>> +if (ret)
>> +return ret;
>> +
>> +/* wait for debug device to be configured */
>> +ret = handshake(_reg->control,
>> +CTRL_DCR, CTRL_DCR, 500, 10);
>> +if (ret)
>> +return ret;
>> +
>> +xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
>> +
>> +xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
>> +
>> +return 0;
>> +}
>> +
> ...
>
>> +static int __init xdbc_early_start(void)
>> +{
>> +u32 ctrl, status;
>> +int ret;
>> +
>> +ctrl = readl(_reg->control);
>> +writel(ctrl | CTRL_DCE | CTRL_PED, _reg->control);
>> +ret = handshake(_reg->control,
>> +CTRL_DCE, CTRL_DCE, 10, 100);
>> +if (ret) {
>> +pr_notice("falied 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(_reg->portsc,
>> +PORTSC_CCS, PORTSC_CCS, 500, 100);
>> +if (ret) {
>> +pr_notice("waiting for connection timed out\n");
>> +return ret;
>> +}
>> +
>> +/* wait for debug device to be configured */
>> +ret = handshake(_reg->control,
>> +CTRL_DCR, CTRL_DCR, 500, 100);
>> +if (ret) {
>> +pr_notice("waiting for device configuration timed out\n");
>> +return ret;
>> +}
>> +
>> +/* port should have a valid port# */
>> +status = readl(_reg->status);
>> +if (!DCST_DPN(status)) {
>> +pr_notice("invalid root hub port number\n");
>> +return -ENODEV;
>> +}
>> +
>> +xdbc.port_number = DCST_DPN(status);
>> +
>> +pr_notice("DbC is running now, control 0x%08x port ID %d\n",
>> +  readl(_reg->control), xdbc.port_number);
>> +
>> +return 0;
>> +}
>
>
> xdbc_early_start() and 

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

[Mathias Nyman]

On 29.08.2016 08:26, Lu Baolu wrote:

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

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

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

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

Cc: Mathias Nyman 
Signed-off-by: Lu Baolu 
---


Some comments inline


+
+#ifdef XDBC_TRACE
+#definexdbc_trace  trace_printk
+#else
+static inline void xdbc_trace(const char *fmt, ...) { }
+#endif /* XDBC_TRACE */


I guess there's probably no better way to enable/disable debug messages for this
driver this early, and ehci-dbgp does the same.

So I guess It's ok, but it still looks weird


+
+static void xdbc_early_delay(unsigned long count)
+{
+   u8 val;
+
+   val = inb(0x80);
+   while (count-- > 0)
+   outb(val, 0x80);
+}
+
+static void xdbc_runtime_delay(unsigned long count)
+{
+   udelay(count);
+}
+


Glad to see the addition of this runtime_delay in addition
to the hack of reading port 0x80 for a 1us delay.

And that the early_delay is only used for as long as it must.



+static int xdbc_handle_external_reset(void)
+{
+   u32 ctrl;
+   int ret;
+
+   writel(0, _reg->control);
+   ret = handshake(_reg->control, CTRL_DCE, 0, 10, 10);
+   if (ret)
+   return ret;
+
+   xdbc_mem_init();
+   mmiowb();
+
+   ctrl = readl(_reg->control);
+   writel(ctrl | CTRL_DCE | CTRL_PED, _reg->control);
+   ret = handshake(_reg->control,
+   CTRL_DCE, CTRL_DCE, 10, 10);
+   if (ret)
+   return ret;
+
+   if (xdbc.vendor == PCI_VENDOR_ID_INTEL)
+   xdbc_do_reset_debug_port(xdbc.port_number, 1);
+
+   /* wait for port connection */
+   ret = handshake(_reg->portsc,
+   PORTSC_CCS, PORTSC_CCS, 500, 10);
+   if (ret)
+   return ret;
+
+   /* wait for debug device to be configured */
+   ret = handshake(_reg->control,
+   CTRL_DCR, CTRL_DCR, 500, 10);
+   if (ret)
+   return ret;
+
+   xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
+
+   xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
+
+   return 0;
+}
+

...


+static int __init xdbc_early_start(void)
+{
+   u32 ctrl, status;
+   int ret;
+
+   ctrl = readl(_reg->control);
+   writel(ctrl | CTRL_DCE | CTRL_PED, _reg->control);
+   ret = handshake(_reg->control,
+   CTRL_DCE, CTRL_DCE, 10, 100);
+   if (ret) {
+   pr_notice("falied 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(_reg->portsc,
+   PORTSC_CCS, PORTSC_CCS, 500, 100);
+   if (ret) {
+   pr_notice("waiting for connection timed out\n");
+   return ret;
+   }
+
+   /* wait for debug device to be configured */
+   ret = handshake(_reg->control,
+   CTRL_DCR, CTRL_DCR, 500, 100);
+   if (ret) {
+   pr_notice("waiting for device configuration timed out\n");
+   return ret;
+   }
+
+   /* port should have a valid port# */
+   status = readl(_reg->status);
+   if (!DCST_DPN(status)) {
+   pr_notice("invalid root hub port number\n");
+   return -ENODEV;
+   }
+
+   xdbc.port_number = DCST_DPN(status);
+
+   pr_notice("DbC is running now, control 0x%08x port ID %d\n",
+ readl(_reg->control), xdbc.port_number);
+
+   return 0;
+}



xdbc_early_start() and xdbc_handle_external_reset() have a lot of duplicate code
, checking DCE, resetting the port, wait for CCE and wait for DCR.

Maybe put the common parts in a separate function?


+static void 

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

[Oliver Neukum]

On Thu, 2016-09-01 at 10:21 +0800, Lu Baolu wrote:
> Hi Oliver,
> 
> Thanks for review.
> 
> On 08/31/2016 05:53 PM, Oliver Neukum wrote:
> > On Mon, 2016-08-29 at 13:26 +0800, Lu Baolu wrote:
> >> +   /*
> >> +* 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);
> > And this manipulation you don't need the hardware to see?
> 
> TRB is a shared memory block between CPU and DbC hardware.
> TRB_CYCLE bit is the identifier of the owner. When CPU submits
> a transfer block to device, it populates the TRB and toggles the
> cycle bit in the end to let device handle it.
> 
> Before toggling the cycle bit, driver must make sure 1) the
> compiler should not reorder the write sequence, and 2) all
> writes should be really take effect in memory instead of pending
> in the caches. That's the reason I put a wmb() before toggling
> the cycle bit.

I see. The comment is a bit misleading.

Sorry
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lu Baolu]

Hi Oliver,

Thanks for review.

On 08/31/2016 05:53 PM, Oliver Neukum wrote:
> On Mon, 2016-08-29 at 13:26 +0800, Lu Baolu wrote:
>> +   /*
>> +* 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);
> And this manipulation you don't need the hardware to see?

TRB is a shared memory block between CPU and DbC hardware.
TRB_CYCLE bit is the identifier of the owner. When CPU submits
a transfer block to device, it populates the TRB and toggles the
cycle bit in the end to let device handle it.

Before toggling the cycle bit, driver must make sure 1) the
compiler should not reorder the write sequence, and 2) all
writes should be really take effect in memory instead of pending
in the caches. That's the reason I put a wmb() before toggling
the cycle bit.

Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

On Mon, 2016-08-29 at 13:26 +0800, Lu Baolu wrote:
> +   /*
> +* 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);

And this manipulation you don't need the hardware to see?

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lu Baolu]

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

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

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

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

Cc: Mathias Nyman 
Signed-off-by: Lu Baolu 
---
 arch/x86/Kconfig.debug|   14 +
 drivers/usb/Kconfig   |3 +
 drivers/usb/Makefile  |2 +-
 drivers/usb/early/Makefile|1 +
 drivers/usb/early/xhci-dbc.c  | 1090 +
 drivers/usb/early/xhci-dbc.h  |  202 
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1333 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..13e85b7 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
bool "Early printk via EHCI debug port"
depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
---help---
  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
  This is useful for kernel debugging when your machine crashes very
  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+   bool "Early printk via xHCI debug port"
+   depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
+   ---help---
+ Write kernel log output directly into the xHCI debug port.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized. For normal operation
+ it is not recommended because it looks ugly and doesn't cooperate
+ with klogd/syslogd or the X server. You should normally N here,
+ unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
def_bool n
 
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..660948b 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 dca7856..dd91ca1 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -48,7 +48,7 @@ obj-$(CONFIG_USB_MICROTEK)+= image/
 obj-$(CONFIG_USB_SERIAL)   += serial/
 
 obj-$(CONFIG_USB)  += misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK) += early/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
index 24bbe51..2db5906 100644
--- a/drivers/usb/early/Makefile
+++ b/drivers/usb/early/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
new file mode 100644
index 000..d7d274f
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1090 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int