* Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> 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.
s/xHCI host/the xHCI host
> 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.
s/out/output
s/in/input
> +config EARLY_PRINTK_USB_XDBC
> + bool "Early printk via xHCI debug port"
s/xHCI/the xHCI
I remarked on this in my first review as well - mind checking the whole series for
uses of 'xHCI'?
> + depends on EARLY_PRINTK && PCI
> + select EARLY_PRINTK_USB
> + ---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 fbe493d..9313fff 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 7791af6..53d1356 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK) += image/
> obj-$(CONFIG_USB_SERIAL) += serial/
>
> obj-$(CONFIG_USB) += misc/
> -obj-$(CONFIG_EARLY_PRINTK_DBGP) += early/
> +obj-$(CONFIG_EARLY_PRINTK_USB) += 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..fcde228 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_USB_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..72480c4
> --- /dev/null
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -0,0 +1,1027 @@
> +/**
> + * xhci-dbc.c - xHCI debug capability early driver
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
> +
> +#include <linux/console.h>
> +#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/module.h>
> +#include <linux/delay.h>
> +
> +#include "../host/xhci.h"
> +#include "xhci-dbc.h"
> +
> +static struct xdbc_state xdbc;
> +static int early_console_keep;
> +
> +#ifdef XDBC_TRACE
> +#define xdbc_trace trace_printk
> +#else
> +static inline void xdbc_trace(const char *fmt, ...) { }
> +#endif /* XDBC_TRACE */
> +
> +static int xdbc_bulk_transfer(void *data, int size, bool read);
> +
> +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);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
> + if (val == 0xffffffff || sz == 0xffffffff) {
> + pr_notice("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;
Is this cast necessary?
> +
> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + 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;
Could these type casts be avoided by declaring 'val' and 'sz' as u64 to begin
with?
> + mask64 |= (u64)~0 << 32;
Type cast could be avoided by using 0L.
> +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);
> + if ((class >> 8) != PCI_CLASS_SERIAL_USB_XHCI)
> + continue;
> +
> + if (xdbc_num-- != 0)
> + continue;
> +
> + *b = bus;
> + *d = dev;
> + *f = func;
> +
> + return 0;
> + }
> + }
> + }
Nit: to me it would look more balanced visually if the body of the innermost loop
started with an empty newline.
> + ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
> + 0, XHCI_EXT_CAPS_LEGACY);
Please don't break this line. You can shorten the variable name if you want to
save line length.
> + /* populate the contexts */
> + context = (struct xdbc_context *)xdbc.dbcc_base;
> + context->info.string0 = cpu_to_le64(xdbc.string_dma);
> + context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
> + context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
> + context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
> + context->info.length = cpu_to_le32(string_length);
Wouldn't this (and some of the other initializations) look nicer this way:
/* Populate the contexts: */
context = (struct xdbc_context *)xdbc.dbcc_base;
context->info.string0 = cpu_to_le64(xdbc.string_dma);
context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
context->info.length = cpu_to_le32(string_length);
?
> + /* Check completion of the previous request. */
Could you please standardize the capitalization and punctuation of all the
comments in the patches? Some are lower case, some are upper case, some have full
stops, some don't.
One good style would be to use this form when a comment refers to what the next
statement does:
/* Check completion of the previous request: */
> +/**
> + * struct xdbc_regs - xHCI Debug Capability Register interface.
> + */
> +struct xdbc_regs {
> + __le32 capability;
> + __le32 doorbell;
> + __le32 ersts; /* Event Ring Segment Table Size*/
> + __le32 __reserved_0; /* 0c~0f reserved bits */
> + __le64 erstba; /* Event Ring Segment Table Base Address */
> + __le64 erdp; /* Event Ring Dequeue Pointer */
> + __le32 control;
> +#define DEBUG_MAX_BURST(p) (((p) >> 16) & 0xff)
> +#define CTRL_DCR BIT(0) /* DbC Run */
> +#define CTRL_PED BIT(1) /* Port Enable/Disable */
> +#define CTRL_HOT BIT(2) /* Halt Out TR */
> +#define CTRL_HIT BIT(3) /* Halt In TR */
> +#define CTRL_DRC BIT(4) /* DbC run change */
> +#define CTRL_DCE BIT(31) /* DbC enable */
> + __le32 status;
> +#define DCST_DPN(p) (((p) >> 24) & 0xff)
> + __le32 portsc; /* Port status and control */
> +#define PORTSC_CCS BIT(0)
> +#define PORTSC_CSC BIT(17)
> +#define PORTSC_PRC BIT(21)
> +#define PORTSC_PLC BIT(22)
> +#define PORTSC_CEC BIT(23)
> + __le32 __reserved_1; /* 2b~28 reserved bits */
> + __le64 dccp; /* Debug Capability Context Pointer */
> + __le32 devinfo1; /* Device Descriptor Info Register 1 */
> + __le32 devinfo2; /* Device Descriptor Info Register 2 */
> +};
So why are defines intermixed with the fields? If the defines relate to the field
then their naming sure does not express that ...
I was thinking of something like:
/**
* struct xdbc_regs - xHCI Debug Capability Register interface.
*/
struct xdbc_regs {
__le32 capability;
__le32 doorbell;
__le32 ersts; /* Event Ring Segment Table Size*/
__le32 __reserved_0; /* 0c~0f reserved bits */
__le64 erstba; /* Event Ring Segment Table Base Address */
__le64 erdp; /* Event Ring Dequeue Pointer */
__le32 control;
__le32 status;
__le32 portsc; /* Port status and control */
__le32 __reserved_1; /* 2b~28 reserved bits */
__le64 dccp; /* Debug Capability Context Pointer */
__le32 devinfo1; /* Device Descriptor Info Register 1 */
__le32 devinfo2; /* Device Descriptor Info Register 2 */
};
#define DEBUG_MAX_BURST(p) (((p) >> 16) & 0xff)
#define CTRL_DCR BIT(0) /* DbC Run */
#define CTRL_PED BIT(1) /* Port Enable/Disable */
#define CTRL_HOT BIT(2) /* Halt Out TR */
#define CTRL_HIT BIT(3) /* Halt In TR */
#define CTRL_DRC BIT(4) /* DbC run change */
#define CTRL_DCE BIT(31) /* DbC enable */
#define DCST_DPN(p) (((p) >> 24) & 0xff)
#define PORTSC_CCS BIT(0)
#define PORTSC_CSC BIT(17)
#define PORTSC_PRC BIT(21)
#define PORTSC_PLC BIT(22)
#define PORTSC_CEC BIT(23)
Also, the abbreviations suck big time. What's wrong with:
#define CTRL_DBC_RUN BIT(0)
#define CTRL_PORT_ENABLE BIT(1)
#define CTRL_HALT_OUT_TR BIT(2)
#define CTRL_HALT_IN_TR BIT(3)
#define CTRL_DBC_RUN_CHANGE BIT(4)
#define CTRL_DBC_ENABLE BIT(31)
?
Also note how the comments became superfluous once descriptive names are chosen...
Please review the rest of the defines and series for similar problems and fix
them, there are more of the same type.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
