Hi Ingo,
On 01/22/2017 05:31 PM, Ingo Molnar wrote:
> * 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.
Thank you for the comments.
I will correct all the code style issues you pointed out here. And, I will
review all patches and fix the similar code styles.
Best regards,
Lu Baolu
--
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
