[usbdev] FIFO state inaccessible to software

[alees24]

Description

Correct operation of the usbdev block relies upon the software knowing the buffer IDs passed to the Available Buffer FIFO (avbuffer) and not using those buffers (queueing them again, or attempting to use them for transmission of IN packets). Software can neither inspect the contents of the avbuffer (which would be tricky), nor flush the FIFO.

In normal use this is fine; my concern is that in the event of suspend/resume, or unexpected bus behavior, there may be a case in which buffer IDs have been supplied to the Available Buffer FIFO but it is difficult/impossible for the software and hardware to recover synchronization without forcing a reset of the 'usbdev' block (via the 'ast' Analog Sensor Top, which is not supposed to be reprogrammed after first ROM boot). The risk would be that the first few Control Transfers from the host to set the device address/configuration would not be properly processed, and the device would not be set up.

RFC:

Since the 'avbuffer' and 'rxfifo' implementations already have synchronous resets ('clr_i') I propose that these should be tied to a software controlled register that would be driven only upon startup/reconfiguration when the device is disconnected and the link inactive.

Even if the implementation is as simple as the register flop driving the signal 'clr_n' directly' and requires a couple of register writes, it could be a valuable contingency measure; the rest of the hardware state is (probably) recoverable already.

Aside: with our current DIF code, it is by this token not possible to call usbdev_configure() a second time after reset because doing so will render the buffer pool inconsistent with the hardware state. If there is any case when the exact PHY configuration (single ended vs differential, swapped vs unswapped pins etc) needs to change during run time, this is a problem.

estimate 8

[a-will]

I don't totally understand the use case, but maybe if we break this down a bit, a discussion will make it clearer.

my concern is that in the event of suspend/resume, or unexpected bus behavior, there may be a case in which buffer IDs have been supplied to the Available Buffer FIFO but it is difficult/impossible for the software and hardware to recover synchronization without forcing a reset of the 'usbdev' block (via the 'ast' Analog Sensor Top

• How would suspend/resume affect the AV FIFO? Am I missing something?
  • For the powered down case, the usbdev gets reset upon waking up, and ordinary initialization causes both sides to be in sync.
  • For the powered up case, the chip doesn't discard any state. Both software and the AV FIFO preserve all the values they had before the link is suspended.
• Software-controlled resets would come from rstmgr, not AST. There is a CSR in top_earlgrey's rstmgr for this purpose.

Since the 'avbuffer' and 'rxfifo' implementations already have synchronous resets ('clr_i') I propose that these should be tied to a software controlled register that would be driven only upon startup/reconfiguration when the device is disconnected and the link inactive.

If the device is disconnected, isn't the reset from rstmgr enough?

These resets seem innocent enough, but we'll want to be careful about adding more controls. More controls leads to more needed testing, maintenance, and documentation.

Aside: with our current DIF code, it is by this token not possible to call usbdev_configure() a second time after reset because doing so will render the buffer pool inconsistent with the hardware state. If there is any case when the exact PHY configuration (single ended vs differential, swapped vs unswapped pins etc) needs to change during run time, this is a problem.

Note that the PHY configuration is solely a property of the chip and the board. These don't actually need to change during run time. Are you seeing a case where there is a need to call dif_usbdev_configure() a second time after (usbdev) reset?

[alees24]

Thanks for the reply. I was unaware/uncertain of the ability to perform a software reset of the usbdev using the rstmgr (searches had only led us locally to the AST). I am a bit concerned about the availability of the rstmgr functionality once things are up and running, and would be happier to have a way to empty those FIFOs from software, as a contingency against the unforeseen, but if the software reset via rstmgr definitely will be available at run time then I guess that should suffice.

Personally if it were up to me alone, I'd wire them up, as a contingency at almost zero hardware cost, even if they're never needed, used or documented. Call my position somewhere between experience and paranoia :)

Re changing the phy config, what about the use case of wiring to USB-C pins SBU1/SBU2 - annotated in the documentation at https://docs.opentitan.org/hw/ip/usbdev/doc/ ?

I don't know much about that use case - whether it's required or exactly how it should work - but the background to the concern about avbuffer/rxfifo state, is that I'm prototyping a t-l test that cycles the usbdev and DPI model through the 8 different bus types (permutations of pinflip, en_diff_rcvr and tx_use_d_se0) and so I'd started to think about disconnect and reinitialising.

[a-will]

Ah, you may have found one: That use case is ChromeOS's Closed-Case Debugging (CCD) setup, where the SBU1 and SBU2 can swap roles with the cable orientation.

However, supporting pin swaps triggered by cable orientations feels like a dubious feature to me. OT would need to get a poke from outside to determine the orientation before being able to raise the correct pull-up. At some point in time, it was acceptable to have to try different orientations for the SuzyQ cable, so such a feature doesn't seem critical.

That said, since usbdev must be disconnected during this negotiation, giving the IP-wide software reset and following up with dif_usbdev_configure() would do the trick (for synchronizing software and hardware).

Edit (much later): This wasn't meant to look like opposition; it's just an explanation of why the feature isn't strictly needed. It doesn't seem a bad idea to have a contingency (in case relying on rstmgr has downsides), aside from this adding to the DV load, hehe.

[moidx]

CC: @toshaq to follow up on product integration requirements. Marking as P3 assuming that if we want to do something about this, it can be handled in firmware.

[jettr]

Having a way to flush and reset the available buffer FIFOs seems like a good idea. We have a couple of TODOs in our implementation to clear buffers in reset/error scenarios. Maybe there is already a workaround/procedure in FW to do that though; I might not be understanding this issue clearly.

@jesultra fyi

[johngt]

@alees24 - please see the comment from @jettr and comment on whether there is a workaround procedure documented. If there is no work-around then we would need to get this in.