Re: [PATCH v7 02/10] usb: dbc: probe and setup xhci debug capability

[Lu Baolu]

Hi Mathias,

Thanks for your time.

On 02/16/2016 10:19 PM, Mathias Nyman wrote:
> Hi
>
> Most of my concerns are due to the early printk restrictions.
> As you say in the documentation:
> "Keep in mind that things such as interrupt, system memory, DMA memory,
> PCI configure space access, etc., are all different from a normal device
> driver."
>
> I don't know the details of these subsystems, or if what is done here is
> done the "right" way.
>
> DMA capable memory is allocated with static char xpage[PAGE_SIZE] __aligned,
> and DMA address is found using  __pa()
>
> xdbc_map_pci_mmio() does some pci magic and fixmapping that I don't know the
> details of.
>
> writing to ioport 0x80 outb(1, 0x80) is used as 1us delay, sometimes up to
> 5 million times to get a 5 second timeout
>
> Adding Bjorn Helgaas and Dan Williams in case they have an opinoin about
> the DMA and PCI cases.

All the quirks in this file have been documented and verified to work.
Your concerns are right, I'll be glad to see other opinions.

>
>
> more comments inline
>
> On 26.01.2016 14:58, Lu Baolu wrote:
>> xHCI debug capability (DbC) is an optional functionality provided
>> by an xHCI host controller. Software learns this capability by
>> walking through the extended capability list in mmio of the host.
>>
>> This patch introduces the code to probe and initialize the debug
>> capability hardware during early boot. With hardware initialization
>> done, the debug target (system under debug which has DbC enabled)
>> will present a debug device through the debug 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.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> ---
>>   MAINTAINERS                  |   7 +
>>   arch/x86/Kconfig.debug       |  12 +
>>   drivers/usb/early/Makefile   |   1 +
>>   drivers/usb/early/xhci-dbc.c | 774 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb/xhci-dbc.h | 187 +++++++++++
>>   5 files changed, 981 insertions(+)
>>   create mode 100644 drivers/usb/early/xhci-dbc.c
>>   create mode 100644 include/linux/usb/xhci-dbc.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 30aca4a..e6d7076 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11505,6 +11505,13 @@ S:    Supported
>>   F:    drivers/usb/host/xhci*
>>   F:    drivers/usb/host/pci-quirks*
>>
>> +USB XHCI DEBUG PORT
>> +M:    Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> +L:    linux-usb@xxxxxxxxxxxxxxx
>> +S:    Supported
>> +F:    drivers/usb/early/xhci-dbc.c
>> +F:    include/linux/usb/xhci-dbc.h
>> +
>>   USB ZD1201 DRIVER
>>   L:    linux-wireless@xxxxxxxxxxxxxxx
>>   W:    http://linux-lc100020.sourceforge.net
>> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
>> index 9b18ed9..ba60cb1 100644
>> --- a/arch/x86/Kconfig.debug
>> +++ b/arch/x86/Kconfig.debug
>> @@ -48,6 +48,18 @@ 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
>> +    ---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/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 0000000..254a0a8
>> --- /dev/null
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -0,0 +1,774 @@
>> +/**
>> + * xhci-dbc.c - xHCI debug capability driver
>> + *
>> + * Copyright (C) 2015 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> + * Some code shared with EHCI debug port and xHCI driver.
>> + *
>> + * 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/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/usb/xhci-dbc.h>
>> +
>> +#include "../host/xhci.h"
>> +
>> +#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 */
>> +
>> +static struct xdbc_state xdbc_stat;
>> +static struct xdbc_state *xdbcp = &xdbc_stat;
>> +
>>
>
> ...
>
>> +
>> +#else
>> +static inline void xdbc_trace(const char *fmt, ...) { }
>> +static inline void xdbc_dbg_dump_regs(char *str) { }
>> +static inline void xdbc_dbg_dump_data(char *str) { }
>> +#endif    /* DBC_DEBUG */
>> +
>> +/*
>> + * FIXME: kernel provided delay interfaces, like usleep, isn't ready yet
>> + *        at the time DbC gets initialized. Below implementation is only
>> + *        for x86 platform. Need to reconsider this when porting it onto
>> + *        other architectures.
>> + */
>> +static inline void xdbc_udelay(int us)
>> +{
>> +    while (us-- > 0)
>> +        outb(0x1, 0x80);
>> +}
>
> This is one of the concerns, the FIXME text describes the problem but still wondering if
> there is any other solution. In many cases later in this file we will wait up to 5 seconds
> for something, ending up writing 5 million times to ioport 0x80.
>
>> +
>> +static void __iomem *xdbc_map_pci_mmio(u32 bus,
>> +        u32 dev, u32 func, u8 bar, size_t *length)
>> +{
>> +    u32 val, sz;
>> +    u64 val64, sz64, mask64;
>> +    u8 byte;
>> +    unsigned long idx, max_idx;
>> +    void __iomem *base;
>> +
>> +    val = read_pci_config(bus, dev, func, bar);
>> +    write_pci_config(bus, dev, func, bar, ~0);
>> +    sz = read_pci_config(bus, dev, func, bar);
>> +    write_pci_config(bus, dev, func, bar, val);
>> +    if (val == 0xffffffff || sz == 0xffffffff) {
>> +        xdbc_trace("invalid mmio bar\n");
>> +        return NULL;
>> +    }
>> +
>> +    val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
>> +    sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
>> +    mask64 = (u32)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, bar + 4);
>> +        write_pci_config(bus, dev, func, bar + 4, ~0);
>> +        sz = read_pci_config(bus, dev, func, bar + 4);
>> +        write_pci_config(bus, dev, func, bar + 4, val);
>> +
>> +        val64 |= ((u64)val << 32);
>> +        sz64 |= ((u64)sz << 32);
>> +        mask64 |= ((u64)~0 << 32);
>> +    }
>> +
>> +    sz64 &= mask64;
>> +
>> +    if (sizeof(dma_addr_t) < 8 || !sz64) {
>> +        xdbc_trace("can't handle 64bit BAR\n");
>> +        return NULL;
>> +    }
>> +
>> +    sz64 = 1ULL << __ffs64(sz64);
>> +
>> +    if (sz64 > (FIX_XDBC_END - FIX_XDBC_BASE + 1) * PAGE_SIZE) {
>> +        xdbc_trace("mmio size beyond 64k not supported\n");
>> +        return NULL;
>> +    }
>> +
>> +    xdbc_trace("bar: base 0x%llx size 0x%llx offset %03x\n",
>> +            val64, sz64, bar);
>> +
>> +    /* 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_trace("mmio for xhci enabled\n");
>> +    }
>> +
>> +    /* 64k mmio will be fix-mapped */
>> +    max_idx = FIX_XDBC_END - FIX_XDBC_BASE;
>> +    for (idx = 0; idx <= max_idx; idx++)
>> +        set_fixmap_nocache(FIX_XDBC_BASE + idx,
>> +            (val64 & PAGE_MASK) + (max_idx - idx) * PAGE_SIZE);
>> +    base = (void __iomem *)__fix_to_virt(FIX_XDBC_END);
>> +    base += val64 & ~PAGE_MASK;
>> +
>> +    /* save in the state block */
>> +    xdbcp->bus = bus;
>> +    xdbcp->dev = dev;
>> +    xdbcp->func = func;
>> +    xdbcp->bar = bar;
>> +    xdbcp->xhci_base = base;
>> +    xdbcp->xhci_length = sz64;
>> +
>> +    if (length)
>> +        *length = sz64;
>> +
>> +    return base;
>> +}
>> +
>> +/*
>> + * FIXME: The bootmem allocator isn't ready at the time when DbC gets
>> + *        initialized. Below implementation reserves DMA memory blocks
>> + *        in the kernel static data segment.
>> + */
>> +static void *xdbc_get_page(dma_addr_t *dma_addr,
>> +        enum xdbc_page_type type)
>> +{
>> +    void *virt;
>> +    static char event_page[PAGE_SIZE] __aligned(PAGE_SIZE);
>> +    static char in_ring_page[PAGE_SIZE] __aligned(PAGE_SIZE);
>> +    static char out_ring_page[PAGE_SIZE] __aligned(PAGE_SIZE);
>> +    static char table_page[PAGE_SIZE] __aligned(PAGE_SIZE);
>> +
>> +    switch (type) {
>> +    case XDBC_PAGE_EVENT:
>> +        virt = (void *)event_page;
>> +        break;
>> +    case XDBC_PAGE_TXIN:
>> +        virt = (void *)in_ring_page;
>> +        break;
>> +    case XDBC_PAGE_TXOUT:
>> +        virt = (void *)out_ring_page;
>> +        break;
>> +    case XDBC_PAGE_TABLE:
>> +        virt = (void *)table_page;
>> +        break;
>> +    default:
>> +        return NULL;
>> +    }
>> +
>> +    memset(virt, 0, PAGE_SIZE);
>> +
>> +    if (dma_addr)
>> +        *dma_addr = (dma_addr_t)__pa(virt);
>> +
>> +    return virt;
>
> another concern, I can't say if this is ok:
>
> void *virt
> static char event_page[PAGE_SIZE] __aligened(PAGE_SIZE)
> virt = (void *)event_page
> *dma_addr = (dma_addr_t)__pa(virt)
>
>> +}
>> +
>> +typedef void (*xdbc_walk_excap_cb)(int cap_offset, void *data);
>> +
>> +/*
>> + * xdbc_walk_excap:
>> + *
>> + * xHCI extended capability list walker.
>> + *
>> + * @bus - xHC PCI bus#
>> + * @dev - xHC PCI dev#
>> + * @func - xHC PCI function#
>> + * @cap - capability ID
>> + * @oneshot - return immediately once hit match
>> + * @cb - call back
>> + * @data - callback private data
>> + *
>> + * Return the last cap offset, otherwize 0.
>> + */
>> +static u32 xdbc_walk_excap(u32 bus, u32 dev, u32 func, int cap,
>> +        bool oneshot, xdbc_walk_excap_cb cb, void *data)
>> +{
>> +    void __iomem *base;
>> +    int offset = 0;
>> +    size_t len = 0;
>> +
>> +    if (xdbcp->xhci_base && xdbcp->xhci_length) {
>> +        if (xdbcp->bus != bus ||
>> +                xdbcp->dev != dev ||
>> +                xdbcp->func != func) {
>> +            xdbc_trace("only one DbC can be used\n");
>> +            return 0;
>> +        }
>> +
>> +        len = xdbcp->xhci_length;
>> +        base = xdbcp->xhci_base;
>> +    } else {
>> +        base = xdbc_map_pci_mmio(bus, dev, func,
>> +                PCI_BASE_ADDRESS_0, &len);
>> +        if (!base)
>> +            return 0;
>> +    }
>> +
>> +    do {
>> +        offset = xhci_find_next_ext_cap(base, offset, cap);
>> +        if (!offset)
>> +            break;
>> +
>> +        if (cb)
>> +            cb(offset, data);
>> +        if (oneshot)
>> +            break;
>> +    } while (1);
>> +
>> +    return offset;
>> +}
>> +
>> +static u32 __init xdbc_find_dbgp(int xdbc_num,
>> +        u32 *rbus, u32 *rdev, u32 *rfunc)
>> +{
>> +    u32 bus, dev, func, class;
>> +    unsigned cap;
>> +
>> +    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;
>> +
>
> Can "for_each_pci_dev()" or similar be used here?  Is it too early for that as well?

Yes. It's too early for "for_each_pci_dev()".

>
>> +                if (xdbc_num-- != 0)
>> +                    continue;
>> +
>> +                cap = xdbc_walk_excap(bus, dev, func,
>> +                        XHCI_EXT_CAPS_DEBUG,
>> +                        true, NULL, NULL);
>> +                *rbus = bus;
>> +                *rdev = dev;
>> +                *rfunc = func;
>> +                return cap;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int handshake(void __iomem *ptr, u32 mask, u32 done,
>> +        int wait_usec, int delay_usec)
>> +{
>> +    u32    result;
>> +
>> +    do {
>> +        result = readl(ptr);
>> +        result &= mask;
>> +        if (result == done)
>> +            return 0;
>> +        xdbc_udelay(delay_usec);
>> +        wait_usec -= delay_usec;
>> +    } while (wait_usec > 0);
>> +
>> +    return -ETIMEDOUT;
>> +}
>> +
>
> ...
>
>> +/*
>> + * xdbc_start: start DbC
>> + *
>> + * Set DbC enable bit and wait until DbC run bit being set or timed out.
>> + */
>> +static int xdbc_start(void)
>> +{
>> +    u32 ctrl, status;
>> +
>> +    ctrl = readl(&xdbcp->xdbc_reg->control);
>> +    writel(ctrl | CTRL_DCE | CTRL_LSE, &xdbcp->xdbc_reg->control);
>> +
>> +    if (handshake(&xdbcp->xdbc_reg->control, CTRL_DCE,
>> +            CTRL_DCE, 100000, 100) < 0) {
>> +        xdbc_trace("falied to initialize hardware\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    /* wait for port connection */
>> +    if (handshake(&xdbcp->xdbc_reg->portsc, PORTSC_CCS,
>> +            PORTSC_CCS, 5000000, 100) < 0) {
>> +        xdbc_trace("waiting for connection timed out\n");
>> +        return -ETIMEDOUT;
>> +    }
>> +    xdbc_trace("port connection detected\n");
>> +
>> +    /* wait for debug device to be configured */
>> +    if (handshake(&xdbcp->xdbc_reg->control, CTRL_DCR,
>> +            CTRL_DCR, 5000000, 100) < 0) {
>> +        xdbc_trace("waiting for device configuration timed out\n");
>> +        return -ETIMEDOUT;
>> +    }
>> +
>
> If I follow this correcly the initialization goes like:
>
> early_xdbc_init()
>   xdbc_setup()
>     write 0 to ctrl reg   // clears (disables/stops) the debug capability.
>     xdbc_start()
>       writel DCE to ctrl reg // this will steal the first port from xhci host driver to debug use.
>       wait max 5 sec for connection (PORTSC_CCS)
>       wait max 5 second for connected host machine to configure this "device" (CTRL_DCR)
>
> Is this debug capability limited to work only when the debug device is rebooted and a separate
> debug host is connected?

Yes. The usage case is very limited. It's used for getting kernel messages at early boot stage.

> If a host connection is not discoved in first 5 seconds, and configured in another 5 seconds, it
> gives up alltogether.
>
> Does this work together with xhci_hcd driver? I think xhci_hcd does a HCRST when driver loads, leading to
> host driver taking back the first port (DCE = 0). Looks like loading xhci_hcd will kill the xdbc early printk.

By default,  all early printk consoles end when a normal console got created. xhci_hcd gets loaded after
that, and HCRST will take back the port. Hence, xdbc early printk doesn't need to work with xhci_hcd.

Early printk has a "keep" option. When "keep" is specified. The early printk boot console will keep alive
in the whole system life. To achieve that with xdbc, there are more work to make it work with xhci_hcd.
That's my next work. Current xdbc early printk will just ignore "keep" option.

My final goal is to make it a reasonable alternative for serial port given the fact that USB 3.0
is more and more popular.

>
> xdbc should probably give back the port if connect / configure fails (set back DCE=0)

Yes. That sounds more reasonable.

>
> -Mathias

Thanks,
Baolu

>
> Leaving some of the initialization code below for reference
>
>
>> +    /* port should have a valid port# */
>> +    status = readl(&xdbcp->xdbc_reg->status);
>> +    if (!DCST_DPN(status)) {
>> +        xdbc_trace("invalid root hub port number\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    xdbc_trace("root hub port number %d\n", DCST_DPN(status));
>> +
>> +    xdbc_trace("DbC is running now, control 0x%08x\n",
>> +            readl(&xdbcp->xdbc_reg->control));
>> +
>> +    return 0;
>> +}
>> +
>> +static int xdbc_setup(void)
>> +{
>> +    int ret;
>> +
>> +    writel(0, &xdbcp->xdbc_reg->control);
>> +    if (handshake(&xdbcp->xdbc_reg->control, CTRL_DCE,
>> +            0, 100000, 100) < 0) {
>> +        xdbc_trace("falied to initialize hardware\n");
>> +        return -ETIMEDOUT;
>> +    }
>> +
>> +    /* allocate and initialize all memory data structures */
>> +    ret = xdbc_mem_init();
>> +    if (ret < 0) {
>> +        xdbc_trace("failed to initialize memory\n");
>> +        return ret;
>> +    }
>> +
>> +    /*
>> +     * Memory barrier to ensure hardware sees the bits
>> +     * setting above.
>> +     */
>> +    mmiowb();
>> +
>> +    /* dump registers and data structures */
>> +    xdbc_dbg_dump_regs("hardware setup completed");
>> +    xdbc_dbg_dump_data("hardware setup completed");
>> +
>> +    ret = xdbc_start();
>> +    if (ret < 0) {
>> +        xdbc_trace("failed to start DbC, cable connected?\n");
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int __init early_xdbc_init(char *s)
>> +{
>> +    u32 bus = 0, dev = 0, func = 0;
>> +    unsigned long dbgp_num = 0;
>> +    u32 offset;
>> +    int ret;
>> +
>> +    if (!early_pci_allowed())
>> +        return -EPERM;
>> +
>> +    /* FIXME: early printk "keep" option will be supported later */
>> +    if (strstr(s, "keep"))
>> +        return -EPERM;
>> +
>> +    if (xdbcp->xdbc_reg)
>> +        return 0;
>> +
>> +    if (*s && kstrtoul(s, 0, &dbgp_num))
>> +        dbgp_num = 0;
>> +
>> +    xdbc_trace("dbgp_num: %lu\n", dbgp_num);
>> +
>> +    offset = xdbc_find_dbgp(dbgp_num, &bus, &dev, &func);
>> +    if (!offset)
>> +        return -ENODEV;
>> +
>> +    xdbc_trace("Found xHCI debug capability on %02x:%02x.%1x\n",
>> +            bus, dev, func);
>> +
>> +    if (!xdbcp->xhci_base)
>> +        return -EINVAL;
>> +
>> +    xdbcp->xdbc_reg = (struct xdbc_regs __iomem *)
>> +            (xdbcp->xhci_base + offset);
>> +    xdbc_dbg_dump_regs("debug capability located");
>> +
>> +    /* hand over the owner of host from BIOS */
>> +    xdbc_bios_handoff();
>> +
>> +    ret = xdbc_setup();
>> +    if (ret < 0) {
>> +        pr_notice("failed to setup xHCI DbC connection\n");
>> +        xdbcp->xhci_base = NULL;
>> +        xdbcp->xdbc_reg = NULL;
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/include/linux/usb/xhci-dbc.h b/include/linux/usb/xhci-dbc.h
>> new file mode 100644
>> index 0000000..153fb87
>> --- /dev/null
>> +++ b/include/linux/usb/xhci-dbc.h
>> @@ -0,0 +1,187 @@
>> +/*
>> + * xHCI debug capability driver
>> + *
>
>