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

[Lu Baolu <baolu.lu@linux.intel.com>]

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@linux.intel.com>
>> ---
>>   .../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@linux.intel.com>
>> +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@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.
>> + */
>> +#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