[opentitan, usbdev] Create initial usbdev block #180

pamaury · 2025-09-10T11:50:49Z

Replaces #164. This creates a minimal implementation of the usbdev block. Currently, the only thing supported are interrupts and also VBUS handling through a dedicated chardev channel. This allows on to pass the usbdev_vbus_test (after some opentitantool changes). Since the usbdev driver will be quite complicated, I think it's better to start small and build up. I also plan to make it multi-file so I created a directory for it.

rivos-eblot

I'm not sure why we want to merge a full dummy device.
It seems for now it does nothing really useful(?)

What does this bring vs. the OT_UNIMP device it replaces?

rivos-eblot · 2025-09-10T12:18:49Z

hw/riscv/ot_earlgrey.c

+            // IBEX_DEV_STRING_PROP("ot_id", "usbdev"),
+            // IBEX_DEV_UINT_PROP("irq-count", 18u),
+            // IBEX_DEV_UINT_PROP("alert-count", 1u),
+            IBEX_DEV_UINT_PROP("pclk", OT_EG_USBDEV_CLK_HZ)


This is an outdated way to configure the clock. USB device should be connected to the clock manager instead (see other peripherals for example, e.g. OT_UART)

I think you may have commented on the old version of the PR? The current version does not have pclk anymore. Can you let me know if the code is correct though?

Oops, sorry I missed your comment. New code seems fine.

include/hw/opentitan/ot_usbdev.h

rivos-eblot · 2025-09-10T12:32:13Z

hw/riscv/ot_earlgrey.c

-        .gpio = IBEXGPIOCONNDEFS(
-            OT_EG_SOC_GPIO_ALERT(0, 21)
-        )
+            // IBEX_DEV_STRING_PROP("ot_id", "usbdev"),


Dead code to be removed

I think this is a comment on the outdated code

I think this is a comment on the outdated code

Please let me know when it is ready to review: even when I refresh I see comments that seem more recent than the actual code which seems to move while I review it :-)

Hum interesting ^^ I will move the PR to draft and set it to ready againa after I fixed everything

hw/riscv/ot_earlgrey.c

rivos-eblot · 2025-09-10T12:34:22Z

hw/riscv/ot_earlgrey.c

 #include "hw/opentitan/ot_sram_ctrl.h"
 #include "hw/opentitan/ot_timer.h"
 #include "hw/opentitan/ot_uart.h"
+#include "hw/opentitan/ot_usbdev.h"


Seems in wrong order, see scripts/opentitan/ot-format.sh

Note that we discovered earlier this week that the CI does not fail when this script is called (it is missing some extra commands that are part of the gitlab CI but did not make it to the github version, my mistake)

hw/opentitan/ot_usbdev.c

pamaury · 2025-09-30T10:00:15Z

hw/riscv/ot_earlgrey.c

+            OT_EG_SOC_GPIO_SYSBUS_IRQ(16, PLIC, 151),
+            OT_EG_SOC_GPIO_SYSBUS_IRQ(17, PLIC, 152),
+            OT_EG_SOC_GPIO_ALERT(0, 21)
+            // TODO Model sense pin


Need to remove this comments, the sense pin is modelled as a real pin because it doesn't make sense for a virtual USB bus.

rivos-eblot · 2025-09-30T10:05:49Z

hw/opentitan/usbdev/ot_usbdev_state.c

@@ -0,0 +1,35 @@
+/*


Can we keep a flat, single file hierarchy? This would be more consisten to all other OT implementation as well as QEMU.

The USB driver will be much too big for a single file I think, unless you are happy with 3000+ lines of code in a single file?

Yes, I think it is better (there are several other files that are longer). There is a couple of exceptions (OTP) which is even longer but has fully-generated C file as it was easier to maintain/upgrade as a separate file. They all stands in the same directory though. We try to keep all IPs with the same structure to it is easier to move from one to another. If the file is getting very big once implemented, we can review this policy and split it (in the same directory) if that makes sense; however for now, I think a single file is ok.

hw/opentitan/usbdev/ot_usbdev_private.h

include/hw/opentitan/ot_usbdev.h

hw/opentitan/usbdev/ot_usbdev_private.h

rivos-eblot · 2025-09-30T10:22:23Z

hw/opentitan/usbdev/ot_usbdev_private.h

+
+/* VBUS control mode. */
+typedef enum OtUsbdevVbusMode {
+    OT_USBDEV_VBUS_OVERRIDE,


I'm not sure to get it, since there is only one value (== 0) in this enum, whatever the value of vbus_mode_str, vbus_mode is always assigned OT_USBDEV_VBUS_OVERRIDE.

It's partly explained in usbdev.md: when we have real USB/IP support, we want to have two modes: one where VBUS is controlled explicitely (independently of the host), and one where VBUS corresponds to host presence (probably the sane default). At the moment we do not have "host" but we still want to control vbus. I can remove that if you want, but it will be added later anyway.

Ok, I get it, although it is a bit weird not to have a defaut non-override value, which could not be selected since you forced it to OVERRIDE through the property anyway.

BTW, as the property is called "vbus-mode", would it not be "override" rather than "vbus-override"? Or since I'm not sure to understand how many "vbus-mode" are planned to be supported, is it not boolean property, i.e. "vbus-override=on/off"?

Yes I think we could make it a boolean property

hw/opentitan/ot_usbdev.c

hw/opentitan/usbdev/ot_usbdev.c

rivos-eblot · 2025-09-30T10:29:49Z

hw/opentitan/ot_usbdev.c

+                          REGS_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+
+    s->link_state = OT_USBDEV_LINK_STATE_DISCONNECTED;


Could be part of the reset handler.
Everything is zero-initialized by QEMU

hw/opentitan/usbdev/ot_usbdev.c

rivos-eblot · 2025-09-30T10:49:42Z

hw/opentitan/usbdev/ot_usbdev.c

+            s->vbus_mode = OT_USBDEV_VBUS_OVERRIDE;
+        }
+        else {
+            error_setg(errp, "%s: %s: invalid VBUS mode: %s", __func__, s->ot_id, s->vbus_mode_str);


Nit pick: Prefer &error_fatal vs errp which defaults to error_abort, which in turn leaves the terminal is an awkward state as it is not properly restored when QEMU abruptly exits :-(

Signed-off-by: Douglas Reis <[email protected]> Signed-off-by: Amaury Pouly <[email protected]>

pamaury requested review from loiclefort and rivos-eblot September 10, 2025 11:50

pamaury changed the base branch from usbdev to ot-earlgrey-9.2.0 September 10, 2025 11:51

pamaury mentioned this pull request Sep 10, 2025

[DRAFT][opentitan, usbdev] Create a dummy usbdev block #164

Closed

pamaury force-pushed the usbdev branch 2 times, most recently from b3d7d26 to 49df0bb Compare September 10, 2025 12:48

rivos-eblot requested changes Sep 10, 2025

View reviewed changes

pamaury marked this pull request as draft September 10, 2025 13:16

pamaury force-pushed the usbdev branch 3 times, most recently from 8048aee to 48bf161 Compare September 16, 2025 12:18

pamaury changed the title ~~[opentitan, usbdev] Create a dummy usbdev block~~ [opentitan, usbdev] Create initial usbdev block Sep 30, 2025

pamaury requested a review from rivos-eblot September 30, 2025 09:52

pamaury force-pushed the usbdev branch 2 times, most recently from 6f5515e to 521a3c0 Compare September 30, 2025 09:56

pamaury commented Sep 30, 2025

View reviewed changes

rivos-eblot reviewed Sep 30, 2025

View reviewed changes

pamaury force-pushed the usbdev branch from 521a3c0 to 9662ba7 Compare September 30, 2025 11:16

[ot] hw/opentitan: ot_usbdev: Initial implementation of the block

3d5a51b

Signed-off-by: Douglas Reis <[email protected]> Signed-off-by: Amaury Pouly <[email protected]>

pamaury force-pushed the usbdev branch from 9662ba7 to 3d5a51b Compare September 30, 2025 13:48

[opentitan, usbdev] Create initial usbdev block #180

Are you sure you want to change the base?

[opentitan, usbdev] Create initial usbdev block #180

Uh oh!

Conversation

pamaury commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pamaury commented Sep 10, 2025 •

edited

Loading