Re: [PATCH v4 01/12] usb: xhci: add sysfs file for xHCI debug port

From: Lu Baolu
Date: Sat Nov 21 2015 - 22:03:02 EST




On 11/20/2015 10:33 PM, Mathias Nyman wrote:
On 17.11.2015 08:38, Lu Baolu wrote:
This patch adds a sysfs file for users to check 1) whether the debug
capability is implemented by hardware; 2) if supported, which state
does it stay at.

With a host that supports debug port, a file named "debug_port_state"
will be created under the device sysfs directory. Reading this file
will show users the state (disabled, enabled or configured) of the
debug port.

With a host that does NOT support debug port, "debug_port_state" file
won't be created.

Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
.../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 23 +++++++
drivers/usb/host/Makefile | 2 +-
drivers/usb/host/xhci-ext-caps.h | 14 +++-
drivers/usb/host/xhci-sysfs.c | 80 ++++++++++++++++++++++
drivers/usb/host/xhci.c | 4 ++
drivers/usb/host/xhci.h | 4 ++
6 files changed, 125 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
create mode 100644 drivers/usb/host/xhci-sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
new file mode 100644
index 0000000..dd3e722
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
@@ -0,0 +1,23 @@
+What: /sys/bus/pci/drivers/xhci_hcd/.../debug_port_state
+Date: November 2015
+KernelVersion: 4.3.0
+Contact: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
+Description:
+ This file is designed for users to check the state of a
+ USB3 debug port. On a machine which supports USB3 debug
+ port, this file will be created. Reading this file will
+ show the state (disabled, enabled or configured) of the
+ debug port. On a machine that doesn't support USB3 debug
+ port, this file doesn't exist.
+
+ The state of a debug port could be:
+ 1) disabled: The debug port is not enabled and the root
+ port has been switched to xHCI host as a normal
+ root port.
+ 2) enabled: The debug port is enabled. The debug port
+ has been assigned to debug capability. The debug
+ capability is able to handle the control requests
+ defined in USB3 spec.
+ 3) configured: The debug port has been enumerated by the
+ debug host as a debug device. The debug port is
+ in use now.

How much will this sysfs file be used, It looks more like debugging info needed while
developing xhci debug port capability.

Would it be enough to add something like "supports DbC" in one of the
dev_info() lines at xhci driver load if capability is supported?

This sysfs file will only be visible after xhci is loaded.
I understood that we would like to use the debug port capability even if xhci driver is not used,
or then for earlyprintk before xhci is loaded.

One use case of this sysfs file is that user can check the DbC status when link
disconnection happens during runtime. User then can recover the link with
another sysfs file which I will implement later.

Some test and debug tools might use DbC as well. Graphic might be an example.
The test applications might want to check the DbC status during runtime.



diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index e7558ab..810c304 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -12,7 +12,7 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o

xhci-hcd-y := xhci.o xhci-mem.o
xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
-xhci-hcd-y += xhci-trace.o
+xhci-hcd-y += xhci-trace.o xhci-sysfs.o

xhci-plat-hcd-y := xhci-plat.o
ifneq ($(CONFIG_USB_XHCI_MVEBU), )
diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
index 9fe3225..12c87e5 100644
--- a/drivers/usb/host/xhci-ext-caps.h
+++ b/drivers/usb/host/xhci-ext-caps.h
@@ -49,8 +49,15 @@
#define XHCI_EXT_CAPS_PM 3
#define XHCI_EXT_CAPS_VIRT 4
#define XHCI_EXT_CAPS_ROUTE 5
-/* IDs 6-9 reserved */
+#define XHCI_EXT_CAPS_LOCALMEM 6
+/* IDs 7-9 reserved */
#define XHCI_EXT_CAPS_DEBUG 10
+/* IDs 192-255 vendor specific */
+#define XHCI_EXT_CAPS_VEN_START 192
+#define XHCI_EXT_CAPS_VEN_END 255

The CAPS_VEN_END is probably not needed,
all capabilities end at 255 (8 bit)

Perhaps just use the EXT_MAX_CAPID defined later

Yes. I forgot to remove it when refactoring the code.


+#define XHCI_EXT_CAPS_VENDOR(p) (((p) >= XHCI_EXT_CAPS_VEN_START) && \
+ ((p) <= XHCI_EXT_CAPS_VEN_END))
+#define XHCI_EXT_MAX_CAPID XHCI_EXT_CAPS_VEN_END
/* USB Legacy Support Capability - section 7.1.1 */
#define XHCI_HC_BIOS_OWNED (1 << 16)
#define XHCI_HC_OS_OWNED (1 << 24)
@@ -73,6 +80,11 @@
#define XHCI_HLC (1 << 19)
#define XHCI_BLC (1 << 20)

+/* Debug capability - section 7.6.8 */
+#define XHCI_DBC_DCCTRL 0x20
+#define XHCI_DBC_DCCTRL_DCR (1 << 0)
+#define XHCI_DBC_DCCTRL_DCE (1 << 31)
+
/* command register values to disable interrupts and halt the HC */
/* start/stop HC execution - do not write unless HC is halted*/
#define XHCI_CMD_RUN (1 << 0)
diff --git a/drivers/usb/host/xhci-sysfs.c b/drivers/usb/host/xhci-sysfs.c
new file mode 100644
index 0000000..365858f
--- /dev/null
+++ b/drivers/usb/host/xhci-sysfs.c
@@ -0,0 +1,80 @@
+/*
+ * sysfs interface for xHCI host controller driver
+ *
+ * Copyright (C) 2015 Intel Corp.
+ *
+ * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
+ *
+ * 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.
+ */
+#include <linux/kernel.h>
+
+#include "xhci.h"
+
+/*
+ * Return the register offset of a extended capability specified
+ * by @cap_id. Return 0 if @cap_id capability is not supported or
+ * in error cases.
+ */
+static int get_extended_capability_offset(struct xhci_hcd *xhci,
+ int cap_id)
+{
+ int offset;
+ void __iomem *base = (void __iomem *) xhci->cap_regs;
+ struct usb_hcd *hcd = xhci_to_hcd(xhci);
+
+ offset = xhci_find_next_cap_offset(base, XHCI_HCC_PARAMS_OFFSET);
+ if (!HCD_HW_ACCESSIBLE(hcd) || !offset)
+ return 0;
+
+ return xhci_find_ext_cap_by_id(base, offset, cap_id);
+}

After looking at how we are parsing the extended capabilities in xhci-mem.c and
pci-quirks.c I see that the current helpers are not that useful.
A new, generic one is needed.

I send a rework out shortly, this way we can avoid adding one more capability
parsing helper for this case.

Agree. Looking forward to the new patch.



-Mathias


Thanks,
Baolu

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/