Hi,
On 09/20/2016 09:54 PM, Mathias Nyman wrote:
> On 29.08.2016 08:26, Lu Baolu 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.
>>
>> 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.
>>
>> This code is designed to be only used for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. For normal operation it is not recommended.
>>
>> Cc: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> ---
>
> Some comments inline
Thank you for reviewing my patch.
>
>> +
>> +#ifdef XDBC_TRACE
>> +#define xdbc_trace trace_printk
>> +#else
>> +static inline void xdbc_trace(const char *fmt, ...) { }
>> +#endif /* XDBC_TRACE */
>
> I guess there's probably no better way to enable/disable debug messages for this
> driver this early, and ehci-dbgp does the same.
>
> So I guess It's ok, but it still looks weird
>
>> +
>> +static void xdbc_early_delay(unsigned long count)
>> +{
>> + u8 val;
>> +
>> + val = inb(0x80);
>> + while (count-- > 0)
>> + outb(val, 0x80);
>> +}
>> +
>> +static void xdbc_runtime_delay(unsigned long count)
>> +{
>> + udelay(count);
>> +}
>> +
>
> Glad to see the addition of this runtime_delay in addition
> to the hack of reading port 0x80 for a 1us delay.
>
> And that the early_delay is only used for as long as it must.
>
>
>> +static int xdbc_handle_external_reset(void)
>> +{
>> + u32 ctrl;
>> + int ret;
>> +
>> + writel(0, &xdbc.xdbc_reg->control);
>> + ret = handshake(&xdbc.xdbc_reg->control, CTRL_DCE, 0, 100000, 10);
>> + if (ret)
>> + return ret;
>> +
>> + xdbc_mem_init();
>> + mmiowb();
>> +
>> + ctrl = readl(&xdbc.xdbc_reg->control);
>> + writel(ctrl | CTRL_DCE | CTRL_PED, &xdbc.xdbc_reg->control);
>> + ret = handshake(&xdbc.xdbc_reg->control,
>> + CTRL_DCE, CTRL_DCE, 100000, 10);
>> + if (ret)
>> + return ret;
>> +
>> + if (xdbc.vendor == PCI_VENDOR_ID_INTEL)
>> + xdbc_do_reset_debug_port(xdbc.port_number, 1);
>> +
>> + /* wait for port connection */
>> + ret = handshake(&xdbc.xdbc_reg->portsc,
>> + PORTSC_CCS, PORTSC_CCS, 5000000, 10);
>> + if (ret)
>> + return ret;
>> +
>> + /* wait for debug device to be configured */
>> + ret = handshake(&xdbc.xdbc_reg->control,
>> + CTRL_DCR, CTRL_DCR, 5000000, 10);
>> + if (ret)
>> + return ret;
>> +
>> + xdbc.flags |= XDBC_FLAGS_INITIALIZED | XDBC_FLAGS_CONFIGURED;
>> +
>> + xdbc_bulk_transfer(NULL, XDBC_MAX_PACKET, true);
>> +
>> + return 0;
>> +}
>> +
> ...
>
>> +static int __init xdbc_early_start(void)
>> +{
>> + u32 ctrl, status;
>> + int ret;
>> +
>> + ctrl = readl(&xdbc.xdbc_reg->control);
>> + writel(ctrl | CTRL_DCE | CTRL_PED, &xdbc.xdbc_reg->control);
>> + ret = handshake(&xdbc.xdbc_reg->control,
>> + CTRL_DCE, CTRL_DCE, 100000, 100);
>> + if (ret) {
>> + pr_notice("falied to initialize hardware\n");
>> + return ret;
>> + }
>> +
>> + /* reset port to avoid bus hang */
>> + if (xdbc.vendor == PCI_VENDOR_ID_INTEL)
>> + xdbc_reset_debug_port();
>> +
>> + /* wait for port connection */
>> + ret = handshake(&xdbc.xdbc_reg->portsc,
>> + PORTSC_CCS, PORTSC_CCS, 5000000, 100);
>> + if (ret) {
>> + pr_notice("waiting for connection timed out\n");
>> + return ret;
>> + }
>> +
>> + /* wait for debug device to be configured */
>> + ret = handshake(&xdbc.xdbc_reg->control,
>> + CTRL_DCR, CTRL_DCR, 5000000, 100);
>> + if (ret) {
>> + pr_notice("waiting for device configuration timed out\n");
>> + return ret;
>> + }
>> +
>> + /* port should have a valid port# */
>> + status = readl(&xdbc.xdbc_reg->status);
>> + if (!DCST_DPN(status)) {
>> + pr_notice("invalid root hub port number\n");
>> + return -ENODEV;
>> + }
>> +
>> + xdbc.port_number = DCST_DPN(status);
>> +
>> + pr_notice("DbC is running now, control 0x%08x port ID %d\n",
>> + readl(&xdbc.xdbc_reg->control), xdbc.port_number);
>> +
>> + return 0;
>> +}
>
>
> xdbc_early_start() and xdbc_handle_external_reset() have a lot of duplicate code
> , checking DCE, resetting the port, wait for CCE and wait for DCR.
>
> Maybe put the common parts in a separate function?
Yes. I agree. Those are duplicated. I will put them in a common function.
>
>> +static void xdbc_scrub_function(struct work_struct *work)
>> +{
>> + unsigned long flags;
>> + int ret;
>> +
>> + /*
>> + * DbC is running, check the event ring and
>> + * handle the events.
>> + */
>> + if (readl(&xdbc.xdbc_reg->control) & CTRL_DRC) {
>> + spin_lock_irqsave(&xdbc.lock, flags);
>> + xdbc_handle_events();
>> + spin_unlock_irqrestore(&xdbc.lock, flags);
>> + }
>> +
>> + /*
>> + * It's time to shutdown DbC, so that the debug
>> + * port could be reused by the host controller.
>> + */
>> + if (unlikely(early_xdbc_console.index == -1 ||
>> + (early_xdbc_console.flags & CON_BOOT))) {
>> + writel(0, &xdbc.xdbc_reg->control);
>> + xdbc_trace("hardware not used any more\n");
>> + return;
>
> Shouldn't we free rings etc here as well?
> we won't queue anymore work after this, right?
Right. I should free all the resources when the hardware will
not be used any more.
>
>> + }
>> +
>> + /*
>> + * External reset happened. Need to restart the
>> + * debugging hardware.
>> + */
>> + if (unlikely(!(readl(&xdbc.xdbc_reg->control) & CTRL_DCE))) {
>> + xdbc.flags &= ~(XDBC_FLAGS_INITIALIZED
>> + | XDBC_FLAGS_CONFIGURED);
>> + ret = xdbc_handle_external_reset();
>> + if (ret) {
>> + xdbc_trace("hardware failed to recover\n");
>> + return;
>
> And here?
The same thing as above.
>
>> + }
>> + }
>> +
>> + queue_delayed_work(xdbc_wq, &xdbc.scrub, usecs_to_jiffies(100));
>> +}
>> +
>
> -Mathias
>
>
>
Thanks for your review again.
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
