ot_usbdev: improve handling of bus resets
#290
Conversation
pamaury
force-pushed
the
usbdev_improvements
branch
from
1ec1de2 to
5f99cda
Compare
pamaury
force-pushed
the
usbdev_improvements
branch
from
5f99cda to
e1836a0
Compare
ziuziakowska
left a comment
ziuziakowska
left a comment
There was a problem hiding this comment.
Not very familiar with the USB but it seems this plays nice with device reset.
| ot_usbdev_update_irqs(s); | ||
|
|
||
| for (unsigned ep = 0; ep < USBDEV_PARAM_N_ENDPOINTS; ep++) { | ||
| /* Cancel any pending IN packets */ | ||
| ot_usbdev_retire_in_packets(s, ep); | ||
| /* Cancel all pending transfers on the server */ | ||
| ot_usbdev_complete_transfer_in(s, ep, OT_USBDEV_SERVER_STATUS_CANCELLED, | ||
| "cancelled by link reset"); | ||
| ot_usbdev_complete_transfer_out(s, ep, | ||
| OT_USBDEV_SERVER_STATUS_CANCELLED, | ||
| "cancelled by link reset"); | ||
| } | ||
| ot_usbdev_cancel_all_transfers(s, "cancelled by link reset"); | ||
|
|
||
| if (ot_usbdev_has_vbus(s) && ot_usbdev_is_enabled(s)) { | ||
| /* | ||
| * @todo BFM has some extra state processing but it seems incorrect | ||
| * @todo If we start tracking frames, this should transition the link | ||
| * state to ACTIVE_NOSOF and then simulate a transition to ACTIVE on | ||
| * the next SOF. | ||
| */ | ||
| ot_usbdev_set_raw_link_state(s, OT_USBDEV_LINK_STATE_ACTIVE); | ||
| } | ||
| ot_usbdev_update_irqs(s); |
There was a problem hiding this comment.
Nit: It doesn't appear that any of these functions here touch the interrupt state, so ot_usbdev_update_irqs is fine anywhere, but it moved which looks a bit confusing.
There was a problem hiding this comment.
The bus reset handling was split into two events: start and end. Previously, the code was simulating both without any delay but now the end has been moved to a timer. But actually only the start triggers an interrupt which is why I moved it closer to the only place where it triggers an IRQ. The end of bus reset signalling does not trigger an interrupt.
pamaury
force-pushed
the
usbdev_improvements
branch
from
e1836a0 to
47121c7
Compare
rivos-eblot
left a comment
rivos-eblot
left a comment
There was a problem hiding this comment.
nit: I missed it in previous review, but the usual order of definitions for OT device usually follow this order:
- properties
- MemoryRegionOps
- reset (enter/hold/exit)
- realize
- init
- class_init
There are a couple of exceptions (that I need to address) for the first devices we wrote before sticking with a defined order. Not a big deal though, just help readibility across devices.
Previously bus resets were emulated as zero-time events. This is not ideal because on a real bus a bus reset takes several milliseconds and this is important to give time to the USB device software stack (otherwise it can lead to confusing situations such as handling a SETUP packet and a bus reset in the same interrupt). This commit now models the bus reset correctly using a timer, splitting the state changes into two steps, closely following the BFM. An important detail is that during the bus signalling, we do not want any other bus events to occur since this could be very confusing. The solution chosen here is that the driver does not accept any new command over the chardev until the bus reset is complete. Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
pamaury
force-pushed
the
usbdev_improvements
branch
from
47121c7 to
b681694
Compare
3 participants
Previously bus resets were emulated as zero-time events. This is not ideal because on a real bus a bus reset takes several milliseconds and this is important to give time to the USB device software stack (otherwise it can lead to confusing situations such as handling a SETUP packet and a bus reset in the same interrupt).
This commit now models the bus reset correctly using a timer, splitting the state changes into two steps, closely following the BFM.
An important detail is that during the bus signalling, we do not want any other bus events to occur since this could be very confusing. The solution chosen here is that the driver does not accept any new command over the chardev until the bus reset is complete.
This PR also fixes some missing transfer cancellations on device reset and vbus loss.
Fixes #28729