gateware.usb2: add disconnect signal to USBResetSequencer

[antoinevg]

This draft PR adds a disconnect signal that, when pulled high by USBDevice, causes the UTMI interface's operating mode to be set to NON_DRIVING in order to trigger a host disconnect.

Open questions:

• Is this the best place to put the logic? e.g. Would it be better to tie it to the USBResetSequencer state machine and only operate if we're in state LS_FS_NON_RESET or HS_NON_RESET?
• I've run into some weirdness where, after data has been transferred on any of the non control endpoints, I can no longer enumerate the device after disconnect/connect. This may however be due to a bug in my code. Currently investigating. It was indeed a software bug.

[miek]

Sorry for the long delay answering.

After going over the spec, I think tying this into the state machine as suggested is the way to go (thought I don't think it needs to be restricted to only work in particular states, after all we're simulating a device being unplugged and that could happen at any time).

According to the spec, there's a minimum time (T_DDIS) that the host needs to see the lines non-driven before detecting a disconnect state, so we should have the state machine guarantee that, and then stay non-driving for longer if the user continues to assert disconnect.

[antoinevg]

Closes #143

[antoinevg]

Updated as per recommendation and everything is working swimmingly. The one surprise was that I needed to transition to START_HS_DETECTION when exiting DISCONNECT rather than INITIALIZE to avoid ending up in a reset loop.

[miek]

Updated as per recommendation and everything is working swimmingly. The one surprise was that I needed to transition to START_HS_DETECTION when exiting DISCONNECT rather than INITIALIZE to avoid ending up in a reset loop.

I think we'll need to find a different solution since low/full speed devices should not transition to that state. Can you expand a bit more on the behaviour when exiting into other states? Resetting the operating mode back to normal upon exiting DISCONNECT might solve it.

[antoinevg]

Heh, I thought you may say something like this -- let's go through it one-by-one:

1. ls/fs devices should not transition to that state

Strictly speaking, yes I agree 100% that this is not ideal. The reason it does work though is that START_HS_DETECTION will transition back to IS_LOW_OR_FULL_SPEED in the event of it being a ls/fs device.

2. Expand a bit more on behaviour when exiting into other states?

I think the root issue with the other states, and why we end up with reset loops with all the alternatives, is that they are all assuming the bus is at reset on entry. This is however not an assumption which is true after a forced disconnect.

Another option I did explore to work around this was to move the SE0 line checks out of LS_FS_NON_RESET and HS_NON_RESET into their own states but I could only get it working in cases where the device speed wasn't changed between disconnect/connect. (a common case with Facedancer emulations) This may however have more to do with my ignorance than the validity of the approach.

I tried a lot of things, but here are some of the failure modes I saw in all cases except the approach used in the PR:

• Never works. Reset loop irrespective of device speed prior or post disconnect.
• Works all the time on ls/fs. Never works on hs.
• Works all the time on hs. Never works on ls/fs.
• Works if device speed stays the same but loops on changing speed.

3. Resetting the operating mode back to normal upon DISCONNECT.

Very first thing I tried… 😅

[antoinevg]

Oh the other thing I wanted to mention was that the existing gateware is pretty much following this approach already if you look at the actual state transitions:

INITIALIZE => LS_FS_NON_RESET from which it will either transition to SUSPENDED or - wait for it - START_HS_DETECTION !

But wait… it gets better:

As written the guard around the transition to START_HS_DETECTION will only block the transition if self.low_speed_only or self.full_speed_only are both high. I don't think it ensures "If we're okay to run in high speed".

Which of course never happens in reality because it's normally either neither or one or the other so of course we will always make that transition irrespective of whether we've configured the device for ls/fs or high speed operation…

The real kicker though is that with the logic -- as written -- it means that if they are ever both set high the gateware will never exit LS_FS_NON_RESET except if the device is suspended so much hilarity would ensue 🤣

Which is pretty much where I found myself last week when I figured we probably have two options at this point:

1. Accept that there may be deeper problems and carefully review the whole of USBResetSequencer with spec in hand.
2. Accept that things are not ideal, but we have empirical evidence for something that works for now so maybe we should merge it and tighten things up when there's a bit more time to put towards it?

[miek]

Having gone back over the state machine with your comments in mind, I've got some thoughts further to the call earlier.

Oh the other thing I wanted to mention was that the existing gateware is pretty much following this approach already if you look at the actual state transitions:

INITIALIZE => LS_FS_NON_RESET from which it will either transition to SUSPENDED or - wait for it - START_HS_DETECTION !

But wait… it gets better:

As written the guard around the transition to START_HS_DETECTION will only block the transition if self.low_speed_only or self.full_speed_only are both high. I don't think it ensures "If we're okay to run in high speed".

This, as far as I can see, is not correct. The guard will block transition if either of them are high (ie: it transitions if self.low_speed_only is low and self.full_speed_only is low). If either are high, it will (and does!) stay in LS_FS_NON_RESET - this is the expected behaviour, since the operating speed/mode has already been set either at reset, in INITIALIZE, or in IS_LOW_OR_FULL_SPEED.

I've verified on the scope, looking at the data lines & the state signal, that it does not currently go through START_HS_DETECTION etc. on a low/full-speed only device, so introducing that behaviour is a hack I'm really not keen on. It happens to work because it goes straight to HS negotiation before the host has issued a reset, so the host isn't yet ready to interpret a device chirp & our device times out waiting for the host chirp, but that's all very fragile.

I've pushed an alternate attempt here: miek@16f9a6c - this sets all the PHY parameters back to expected defaults before going through INITIALIZE, and it seems to be working OK for me:

[272864.421992] usb 1-6.2.3.1: new high-speed USB device number 38 using xhci_hcd
[272864.530079] usb 1-6.2.3.1: config 1 interface 0 altsetting 0 bulk endpoint 0x1 has invalid maxpacket 64
[272864.530083] usb 1-6.2.3.1: config 1 interface 0 altsetting 0 bulk endpoint 0x81 has invalid maxpacket 64
[272864.530238] usb 1-6.2.3.1: New USB device found, idVendor=16d0, idProduct=0f3b, bcdDevice= 0.00
[272864.530240] usb 1-6.2.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[272864.530240] usb 1-6.2.3.1: Product: Test Device
[272864.530241] usb 1-6.2.3.1: Manufacturer: LUNA
[272864.530242] usb 1-6.2.3.1: SerialNumber: 1234
[272867.769013] usb 1-6.2.3.1: USB disconnect, device number 38
[272882.081335] usb 1-6.2.3.1: new full-speed USB device number 39 using xhci_hcd
[272882.182614] usb 1-6.2.3.1: New USB device found, idVendor=16d0, idProduct=0f3b, bcdDevice= 0.00
[272882.182622] usb 1-6.2.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[272882.182625] usb 1-6.2.3.1: Product: Test Device
[272882.182628] usb 1-6.2.3.1: Manufacturer: LUNA
[272882.182631] usb 1-6.2.3.1: SerialNumber: 1234

(low-speed can be selected too, but it errors out because the descriptors are all wrong).

Tested with examples/usb/simple_device.py and the connect/speed signals brought out to the PMODs:

$ git diff
diff --git a/examples/usb/simple_device.py b/examples/usb/simple_device.py
index 325500b..46f413b 100755
--- a/examples/usb/simple_device.py
+++ b/examples/usb/simple_device.py
@@ -9,6 +9,7 @@ import os
 
 from amaranth                       import Elaboratable, Module
 from usb_protocol.emitters          import DeviceDescriptorCollection
+from amaranth.build import *
 
 from luna                           import top_level_cli
 from luna.gateware.platform         import NullPin
@@ -72,10 +73,18 @@ class USBDeviceExample(Elaboratable):
         descriptors = self.create_descriptors()
         usb.add_standard_control_endpoint(descriptors)
 
+        platform.add_resources([
+            Resource("btns", 0, Pins("1 2 3", conn=("pmod", 0), dir="i"), Attrs(PULLMODE="UP")),
+        ])
+
+        btns = platform.request("btns", 0)
+        pmod1 = platform.request("user_pmod", 1, dir="o")
+
         # Connect our device as a high speed device by default.
         m.d.comb += [
-            usb.connect          .eq(1),
-            usb.full_speed_only  .eq(1 if os.getenv('LUNA_FULL_ONLY') else 0),
+            usb.connect          .eq(btns.i[0]),
+            usb.full_speed_only  .eq(btns.i[1]),
+            usb.low_speed_only   .eq(btns.i[2]),
         ]
 
         # ... and for now, attach our LEDs to our most recent control request.

[antoinevg]

Ah you absolute bloody genius you @miek!

Setting self.termination_select.eq(1) was the missing piece from everything else I tried!

I've updated the PR with your changes, thank you.

[martinling]

I have been trying to test this work as part of my HITL work in greatscottgadgets/cynthion-analyzer#6, but I'm having what may or may not be unrelated issues.

[martinling]

I'm now using this to disconnect and reconnect a device successfully at all speeds, so functionally I think it's good to go.