Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Dmitry Malkin]

On Mon, 9 Nov 2015 15:38:33 +0800, Lu Baolu wrote:
> On Intel platform, if the debug target is connected with debug
> host, enabling DCE bit in command register leads to a hung bus
> state. In the hung state, the host system will not see a port
> connected status bit set. Hence debug target fails to be probed.
>
> The state could be resolved by performing a port reset to the
> debug port from the host xHCI. This patch introduces this work
> around.

Is it correct to call this a "hung bus state"?  Wouldn't calling it
"hung port state" more appropriate?

I have observed this DCE-enable-related hung port state,
but the reason seemed to be different in my case:

Citing a note (sic!) from The Holy XHCI Spec, section 7.6.4.1:
> If a Debug Host attempts to attach to a Debug Target before the DCE flag is set,
> both ends of the link shall transition to the Inactive state.
> So a Debug Host should periodically issue a Warm Reset
> to ports that are Inactive to enable a connection to the DbC of the Debug Target.

Indeed, the inactive state is what I have observed (PLS field of port register PORTSC)
when the DCE bit is set to 1 with the cable already plugged in.

Now, according to my interpretation of The Hole USB 3.1 spec, section 7.5.2,
which says:
> eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
> A downstream port can only exit from this state when directed, or upon detection of
> an absence of a far-end receiver termination (R RX-DC ) specified in Table 6-21,
> or upon a Warm Reset.

It follows, that since hosts without DBC cannot listen to upstream requests,
the debug target-originated port reset requests (both hot and warm) will be ignored
by the debug host.

This is the essence of the "hung port state" that I was able to observe.

Note, that this roadblock doesn't appear if you attach the cable /after/ enabling the DCE bit,
or, alternatively, if the host has DBC.

And indeed, your quirk will work in the latter case, since the debug host hub
will be able to see the upstream reset request.

-- 
with best regards,
Dmitry Malkin

Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Dmitry Malkin]

On Mon, 9 Nov 2015 15:38:33 +0800, Lu Baolu wrote:
> On Intel platform, if the debug target is connected with debug
> host, enabling DCE bit in command register leads to a hung bus
> state. In the hung state, the host system will not see a port
> connected status bit set. Hence debug target fails to be probed.
>
> The state could be resolved by performing a port reset to the
> debug port from the host xHCI. This patch introduces this work
> around.

Is it correct to call this a "hung bus state"?  Wouldn't calling it
"hung port state" more appropriate?

I have observed this DCE-enable-related hung port state,
but the reason seemed to be different in my case:

Citing a note (sic!) from The Holy XHCI Spec, section 7.6.4.1:
> If a Debug Host attempts to attach to a Debug Target before the DCE flag is set,
> both ends of the link shall transition to the Inactive state.
> So a Debug Host should periodically issue a Warm Reset
> to ports that are Inactive to enable a connection to the DbC of the Debug Target.

Indeed, the inactive state is what I have observed (PLS field of port register PORTSC)
when the DCE bit is set to 1 with the cable already plugged in.

Now, according to my interpretation of The Hole USB 3.1 spec, section 7.5.2,
which says:
> eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
> A downstream port can only exit from this state when directed, or upon detection of
> an absence of a far-end receiver termination (R RX-DC ) specified in Table 6-21,
> or upon a Warm Reset.

It follows, that since hosts without DBC cannot listen to upstream requests,
the debug target-originated port reset requests (both hot and warm) will be ignored
by the debug host.

This is the essence of the "hung port state" that I was able to observe.

Note, that this roadblock doesn't appear if you attach the cable /after/ enabling the DCE bit,
or, alternatively, if the host has DBC.

And indeed, your quirk will work in the latter case, since the debug host hub
will be able to see the upstream reset request.

--
with best regards,
Dmitry Malkin

Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Lu Baolu]

Hi,

On 11/13/2015 11:34 PM, Dmitry Malkin wrote:
> On Mon, 9 Nov 2015 15:38:33 +0800, Lu Baolu wrote:
>> On Intel platform, if the debug target is connected with debug
>> host, enabling DCE bit in command register leads to a hung bus
>> state. In the hung state, the host system will not see a port
>> connected status bit set. Hence debug target fails to be probed.
>>
>> The state could be resolved by performing a port reset to the
>> debug port from the host xHCI. This patch introduces this work
>> around.
> Is it correct to call this a "hung bus state"?  Wouldn't calling it
> "hung port state" more appropriate?

Yes, "hung port state" is more appropriate.

>
> I have observed this DCE-enable-related hung port state,
> but the reason seemed to be different in my case:
>
> Citing a note (sic!) from The Holy XHCI Spec, section 7.6.4.1:
>> If a Debug Host attempts to attach to a Debug Target before the DCE flag is set,
>> both ends of the link shall transition to the Inactive state.
>> So a Debug Host should periodically issue a Warm Reset
>> to ports that are Inactive to enable a connection to the DbC of the Debug Target.

Exactly. A formal workaround is to periodically issue Warm Resets to ports
from debug host.

> Indeed, the inactive state is what I have observed (PLS field of port register PORTSC)
> when the DCE bit is set to 1 with the cable already plugged in.
>
> Now, according to my interpretation of The Hole USB 3.1 spec, section 7.5.2,
> which says:
>> eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
>> A downstream port can only exit from this state when directed, or upon detection of
>> an absence of a far-end receiver termination (R RX-DC ) specified in Table 6-21,
>> or upon a Warm Reset.
> It follows, that since hosts without DBC cannot listen to upstream requests,
> the debug target-originated port reset requests (both hot and warm) will be ignored
> by the debug host.

As above, a formal workaround is to issue Warm Reset periodically from debug
host, but that could cause much complexity. My quirk is to issue port 
reset just
*before* setting DCE bit. This quirk seems to work on several platforms.
(My debug host doesn't have DBC.)

>
> This is the essence of the "hung port state" that I was able to observe.
>
> Note, that this roadblock doesn't appear if you attach the cable /after/ enabling the DCE bit,

Yes.

> or, alternatively, if the host has DBC.

Really? I haven't tried it yet.

>
> And indeed, your quirk will work in the latter case, since the debug host hub
> will be able to see the upstream reset request.

This quirk works as well if debug host doesn't have DBC. I didn't try a 
DBC-capable
debug host yet.

>
> --
> with best regards,
> Dmitry Malkin

Thank you.
-Baolu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Dmitry Malkin]

On Mon, 16 Nov 2015 10:18:42 +0800, Lu Baolu wrote:
> This quirk works as well if debug host doesn't have DBC. I didn't try a
> DBC-capable debug host yet.

Hi,

I went through it again, with your v3 patch series on top of vanilla v4.3.0.

Two targets, one host, all with Intel chipset (XHCI device 0:14.0 with VID:8086).
Targets DIDs are 9c31 (8-series chipset) and 9d2f (100-series chipset).
Host DID is 8c31 (8-series).

I can only further confirm my previous observations.  The host cannot
see the target (there is no debug device) at all, until I manually
re-plug a debug cable.  Cable plugged in, prior to starting the target.

I think that trying a Hot Reset for all disconnected USB 3.0 ports on
the debug target *just before* setting DCE bit is inherently racy.

What if the number of disconnected ports changes or if someone will insert
a print statement or refactors your code?

Also sending TS2 to the debug host port will either:
 - get dropped by its hub as unsupported upstream request, or
 - get ignored due to SS.Inactive port state

Could you explain what exactly you workaround does at the low level?

--
with best regards,
Dmitry Malkin

Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Lu Baolu]

Hi,

On 11/17/2015 06:28 PM, Dmitry Malkin wrote:
> On Mon, 16 Nov 2015 10:18:42 +0800, Lu Baolu wrote:
>> This quirk works as well if debug host doesn't have DBC. I didn't try a
>> DBC-capable debug host yet.
> Hi,
>
> I went through it again, with your v3 patch series on top of vanilla v4.3.0.
>
> Two targets, one host, all with Intel chipset (XHCI device 0:14.0 with VID:8086).
> Targets DIDs are 9c31 (8-series chipset) and 9d2f (100-series chipset).
> Host DID is 8c31 (8-series).
>
> I can only further confirm my previous observations.  The host cannot
> see the target (there is no debug device) at all, until I manually
> re-plug a debug cable.  Cable plugged in, prior to starting the target.

Thank you for your time.

Are you using a "USB3 debugging cable"? The internal wiring of
the debug cable is crossed over.

> I think that trying a Hot Reset for all disconnected USB 3.0 ports on
> the debug target *just before* setting DCE bit is inherently racy.
>
> What if the number of disconnected ports changes or if someone will insert
> a print statement or refactors your code?

That might be problematic. I will put a comment there.

>
> Also sending TS2 to the debug host port will either:
>   - get dropped by its hub as unsupported upstream request, or
>   - get ignored due to SS.Inactive port state
>
> Could you explain what exactly you workaround does at the low level?

What I though was that reset USB 3.0 ports on debug target before setting
DCE bit will cause debug host to warm reset the port for enumeration
purpose and then setting the DCE bit will take effect.

This just works on my development machine, not only connecting debug
target to root port of host, but also under external hubs. I realized that
this quirk isn't a universal solution and it might not work with some
silicons. But I thought it shouldn't do any harmless. In bad case, users
could plug out and plug in the debug cable, or reset the port through
the sysfs node for workaround. If this is acceptable, I will add this in
the user guide and increase the timeout value in code.

Thanks,
-Baolu

>
> --
> with best regards,
> Dmitry Malkin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Lu Baolu]

On Intel platform, if the debug target is connected with debug
host, enabling DCE bit in command register leads to a hung bus
state. In the hung state, the host system will not see a port
connected status bit set. Hence debug target fails to be probed.

The state could be resolved by performing a port reset to the
debug port from the host xHCI. This patch introduces this work
around.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/early/xhci-dbc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/xhci-dbc.h |  2 ++
 2 files changed, 54 insertions(+)

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index 22a1de9..6b23f09 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -255,6 +255,8 @@ static void __iomem *xdbc_map_pci_mmio(u32 bus,
 	xdbcp->bar = bar;
 	xdbcp->xhci_base = base;
 	xdbcp->xhci_length = sz64;
+	xdbcp->vendor = read_pci_config_16(bus, dev, func, PCI_VENDOR_ID);
+	xdbcp->device = read_pci_config_16(bus, dev, func, PCI_DEVICE_ID);
 
 	if (length)
 		*length = sz64;
@@ -651,6 +653,52 @@ static int xdbc_mem_init(void)
 	return 0;
 }
 
+static void xdbc_reset_debug_port_callback(int cap_offset, void *data)
+{
+	u8 major;
+	u32 val, port_offset, port_count;
+	u32 cap_length;
+	void __iomem *ops_reg;
+	void __iomem *portsc;
+	int i;
+
+	val = readl(xdbcp->xhci_base + cap_offset);
+	major = (u8) XHCI_EXT_PORT_MAJOR(val);
+
+	/* only reset super-speed port */
+	if (major != 0x3)
+		return;
+
+	val = readl(xdbcp->xhci_base + cap_offset + 8);
+	port_offset = XHCI_EXT_PORT_OFF(val);
+	port_count = XHCI_EXT_PORT_COUNT(val);
+	xdbc_trace("Extcap Port offset %d count %d\n",
+			port_offset, port_count);
+
+	cap_length = readl(xdbcp->xhci_base) & 0xff;
+	ops_reg = xdbcp->xhci_base + cap_length;
+
+	port_offset--;
+	for (i = port_offset; i < (port_offset + port_count); i++) {
+		portsc = ops_reg + 0x400 + i * 0x10;
+		val = readl(portsc);
+		/* reset the port if CCS bit is cleared */
+		if (!(val & 0x1))
+			writel(val | (1 << 4), portsc);
+	}
+}
+
+static void xdbc_reset_debug_port(void)
+{
+	xdbc_walk_excap(xdbcp->bus,
+			xdbcp->dev,
+			xdbcp->func,
+			XHCI_EXT_CAPS_PROTOCOL,
+			false,
+			xdbc_reset_debug_port_callback,
+			NULL);
+}
+
 /*
  * xdbc_start: start DbC
  *
@@ -669,6 +717,10 @@ static int xdbc_start(void)
 		return -ENODEV;
 	}
 
+	/* reset port to avoid bus hang */
+	if (xdbcp->vendor == PCI_VENDOR_ID_INTEL)
+		xdbc_reset_debug_port();
+
 	/* wait for port connection */
 	if (handshake(&xdbcp->xdbc_reg->portsc, PORTSC_CCS,
 			PORTSC_CCS, 5000000, 100) < 0) {
diff --git a/include/linux/usb/xhci-dbc.h b/include/linux/usb/xhci-dbc.h
index 153fb87..fc0ef9a 100644
--- a/include/linux/usb/xhci-dbc.h
+++ b/include/linux/usb/xhci-dbc.h
@@ -128,6 +128,8 @@ struct xdbc_state {
 	u32		dev;
 	u32		func;
 	u8		bar;
+	u16		vendor;
+	u16		device;
 	void __iomem	*xhci_base;
 	size_t		xhci_length;
 #define	XDBC_PCI_MAX_BUSES		256
-- 
2.1.4