Skip to content

ot_usbdev: improve handling of bus resets #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: ot-9.2.0
Choose a base branch
from
+84 −23
Open

ot_usbdev: improve handling of bus resets #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d59fb7
[ot] hw/opentitan: ot_usbdev: improve handling of bus resets
pamaury Nov 12, 2025
b681694
[ot] hw/opentitan: ot_usbdev: cancel all transfers on reset/disconnect
pamaury Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 83 additions & 22 deletions hw/opentitan/ot_usbdev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "qemu/log.h"
#include "chardev/char-fe.h"
#include "hw/opentitan/ot_alert.h"
#include "hw/opentitan/ot_common.h"
#include "hw/opentitan/ot_fifo32.h"
#include "hw/opentitan/ot_usbdev.h"
#include "hw/qdev-properties-system.h"
Expand All @@ -63,6 +64,9 @@
#define OT_USBDEV_MAX_PACKET_SIZE 64u
#define OT_USBDEV_BUFFER_COUNT 32u

/* Time in milliseconds for a bus reset to complete. */
#define USBDEV_BUS_RESET_TIME_MS 10

/* Standard constants */
#define OT_USBDEV_SETUP_PACKET_SIZE 8u

Expand Down Expand Up @@ -527,6 +531,11 @@ struct OtUsbdevState {
CharBackend usb_chr;
OtUsbdevServer usb_server;

/* Timer to handle bus reset. */
QEMUTimer bus_reset_timer;
/* A bus reset is in progress */
bool bus_reset_in_progress;

char *ot_id;
/* Link to the clkmgr. */
DeviceState *clock_src;
Expand Down Expand Up @@ -606,6 +615,8 @@ static void ot_usbdev_complete_transfer_out(OtUsbdevState *s, uint8_t epnum,
OtUsbdevServerTransferStatus status,
const char *msg);
static bool ot_usbdev_is_enabled(const OtUsbdevState *s);
static void
ot_usbdev_cancel_all_transfers(OtUsbdevState *s, const char *reason);

/*
* Update the INTR_STATE register for status interrupts.
Expand All @@ -630,6 +641,10 @@ static void ot_usbdev_update_status_irqs(OtUsbdevState *s)
set_mask |= USBDEV_INTR_PKT_RECEIVED_MASK;
}
}
/*
* @todo The BFM says that 'Disconnected' is held at 1 during IP
* reset, need to investigate this detail.
*/
s->regs[R_USBDEV_INTR_STATE] |= set_mask;
}

Expand Down Expand Up @@ -991,6 +1006,8 @@ static void ot_usbdev_update_vbus(OtUsbdevState *s)
s->regs[R_USBDEV_INTR_STATE] |= USBDEV_INTR_DISCONNECTED_MASK;
s->regs[R_USBCTRL] =
FIELD_DP32(s->regs[R_USBCTRL], USBCTRL, DEVICE_ADDRESS, 0u);

ot_usbdev_cancel_all_transfers(s, "VBUS was disconnected");
}

ot_usbdev_update_irqs(s);
Expand Down Expand Up @@ -1030,6 +1047,22 @@ static void ot_usbdev_retire_in_packets(OtUsbdevState *s, uint8_t ep)
s->regs[R_CONFIGIN_0 + ep] = configin;
}

/*
* Callback to notify bus reset completion. The timer is started
* in ot_usbdev_simulate_link_reset.
*/
static void ot_usbdev_link_reset_complete(void *opaque)
{
OtUsbdevState *s = opaque;
s->bus_reset_in_progress = false;

trace_ot_usbdev_link_reset(s->ot_id, true);

if (ot_usbdev_has_vbus(s) && ot_usbdev_is_enabled(s)) {
ot_usbdev_set_raw_link_state(s, OT_USBDEV_LINK_STATE_ACTIVE_NOSOF);
}
}

/*
* Simulate a link reset.
*
Expand All @@ -1039,8 +1072,9 @@ static void ot_usbdev_retire_in_packets(OtUsbdevState *s, uint8_t ep)
static void ot_usbdev_simulate_link_reset(OtUsbdevState *s)
{
g_assert(!resettable_is_in_reset(OBJECT(s)));
g_assert(!s->bus_reset_in_progress);

trace_ot_usbdev_link_reset(s->ot_id);
trace_ot_usbdev_link_reset(s->ot_id, false);

/* We cannot simulate a reset if the device is disconnected! */
if (ot_usbdev_get_link_state(s) == OT_USBDEV_LINK_STATE_DISCONNECTED) {
Expand All @@ -1053,28 +1087,22 @@ static void ot_usbdev_simulate_link_reset(OtUsbdevState *s)
s->regs[R_USBCTRL] =
FIELD_DP32(s->regs[R_USBCTRL], USBCTRL, DEVICE_ADDRESS, 0u);
s->regs[R_USBDEV_INTR_STATE] |= USBDEV_INTR_LINK_RESET_MASK;
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);
Comment on lines +1090 to -1077
Copy link

@ziuziakowska ziuziakowska Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@pamaury pamaury Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/*
* On real hardware, reset signalling typically takes several milliseconds
* so we kick a timer to trigger the link state change after the end of
* signalling.
*/
s->bus_reset_in_progress = true;
int64_t now = qemu_clock_get_ms(OT_VIRTUAL_CLOCK);
timer_mod(&s->bus_reset_timer, now + USBDEV_BUS_RESET_TIME_MS);
}

/*
Expand Down Expand Up @@ -1229,6 +1257,21 @@ void ot_usbdev_complete_transfer_out(OtUsbdevState *s, uint8_t epnum,
memset(xfer, 0, sizeof(OtUsbdevServerEpOutXfer));
}

/*
* Cancel all pending transfers
*/
void ot_usbdev_cancel_all_transfers(OtUsbdevState *s, const char *reason)
{
for (unsigned ep = 0; ep < USBDEV_PARAM_N_ENDPOINTS; ep++) {
/* Cancel all pending transfers on the server */
ot_usbdev_complete_transfer_in(s, ep, OT_USBDEV_SERVER_STATUS_CANCELLED,
reason);
ot_usbdev_complete_transfer_out(s, ep,
OT_USBDEV_SERVER_STATUS_CANCELLED,
reason);
}
}

/*
* Try to make progress with an IN transfer on this endpoint.
*
Expand Down Expand Up @@ -2337,6 +2380,14 @@ static int ot_usbdev_chr_usb_can_receive(void *opaque)
{
OtUsbdevState *s = opaque;

/*
* If bus reset signalling is in progress, delay
* reception until it is done.
*/
if (s->bus_reset_in_progress) {
return 0;
}

/* Return remaining size in the buffer. */
return (int)s->usb_server.recv_rem;
}
Expand Down Expand Up @@ -2525,7 +2576,17 @@ static void ot_usbdev_reset_enter(Object *obj, ResetType type)
c->parent_phases.enter(obj, type);
}

/* @todo cancel everything here. */
ot_usbdev_cancel_all_transfers(s, "device going into reset");

/*
* If there is a bus reset in progress, we cancel it.
* The rationale is that on a real bus, the device
* reset would cause the block to disable the pullup
* so the host would notice the disconnection and stop
* reset signalling.
*/
timer_del(&s->bus_reset_timer);
s->bus_reset_in_progress = false;

/* See BFM (dut_reset) */
memset(s->regs, 0u, sizeof(s->regs));
Expand All @@ -2534,10 +2595,7 @@ static void ot_usbdev_reset_enter(Object *obj, ResetType type)
fifo8_reset(&s->av_setup_fifo);
fifo8_reset(&s->av_out_fifo);

/*
* @todo The BFM says that 'Disconnected' is held at 1 during IP
* reset, need to investigate this detail.
*/
ot_usbdev_update_status_irqs(s);
ot_usbdev_update_irqs(s);
}

Expand Down Expand Up @@ -2580,6 +2638,9 @@ static void ot_usbdev_init(Object *obj)
ot_fifo32_create(&s->rx_fifo, OT_USBDEV_RX_FIFO_DEPTH);
fifo8_create(&s->av_out_fifo, OT_USBDEV_AV_OUT_FIFO_DEPTH);
fifo8_create(&s->av_setup_fifo, OT_USBDEV_AV_SETUP_FIFO_DEPTH);

timer_init_ms(&s->bus_reset_timer, OT_VIRTUAL_CLOCK,
&ot_usbdev_link_reset_complete, s);
}

static void ot_usbdev_class_init(ObjectClass *klass, void *data)
Expand Down
2 changes: 1 addition & 1 deletion hw/opentitan/trace-events
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ ot_usbdev_io_buffer_write(const char *id, uint32_t addr, uint32_t val, uint32_t
ot_usbdev_io_read_out(const char *id, uint32_t addr, const char *regname, uint32_t val, uint32_t pc) "%s: addr=0x%02x (%s), val=0x%x, pc=0x%x"
ot_usbdev_io_write(const char *id, uint32_t addr, const char *regname, uint32_t val, uint32_t pc) "%s: addr=0x%02x (%s), val=0x%x, pc=0x%x"
ot_usbdev_irqs(const char *id, uint32_t active, uint32_t mask, uint32_t eff) "%s: act:0x%08x msk:0x%08x eff:0x%08x"
ot_usbdev_link_reset(const char *id) "%s"
ot_usbdev_link_reset(const char *id, bool completed) "%s: completed=%u"
ot_usbdev_packet_received(const char *id, uint8_t ep, uint8_t bufid, uint32_t size) "%s: ep=%u, buf_id=%u, size=%u"
ot_usbdev_packet_sent(const char *id, uint8_t ep, uint8_t bufid, uint32_t size) "%s: ep=%u, buf_id=%u, size=%u"
ot_usbdev_link_state_changed(const char *id, const char *state) "%s: link_state=%s"
Expand Down
Loading