Skip to content

drivers: usb: add generic ohci host driver#107065

Open
maass-hamburg wants to merge 6 commits intozephyrproject-rtos:mainfrom
maass-hamburg:ohci-generic-host-driver
Open

drivers: usb: add generic ohci host driver#107065
maass-hamburg wants to merge 6 commits intozephyrproject-rtos:mainfrom
maass-hamburg:ohci-generic-host-driver

Conversation

@maass-hamburg
Copy link
Copy Markdown
Member

Introduce support for the Generic OHCI USB host controller driver and add necessary configurations for QEMU boards. Include tests for USB OHCI functionality on QEMU.

@maass-hamburg maass-hamburg changed the title drivers: usb: add gerneric ohci host driver drivers: usb: add generic ohci host driver Apr 13, 2026
@kartben kartben added the AI-assisted At least one commit in the PR has an "Assisted-by: " entry. label Apr 20, 2026
@josuah
Copy link
Copy Markdown
Contributor

josuah commented Apr 25, 2026

Thank you for adding support for Qemu testing.

I see tests added. I propose these two PRs:

If this makes sense, it is possible to rebase this current PR on top of either.

Feedback welcome.

@maass-hamburg
Copy link
Copy Markdown
Member Author

The really working in qemu version is in #107279. I have not yet updated the commits in here.

Josuah Demangeon added 2 commits May 4, 2026 12:18
When zephyr_udc0 or zephyr_uhc0 are enabled, add the more recent usbd
and usbh tags meant to replace usb_device and usb_host.

Signed-off-by: Josuah Demangeon <josuah.demangeon@nordicsemi.no>
Add a simple test that expects a device already connected
when the controller initializes, then read its descriptors.

Signed-off-by: Josuah Demangeon <josuah.demangeon@nordicsemi.no>
@maass-hamburg maass-hamburg force-pushed the ohci-generic-host-driver branch from f0f7207 to bb131ca Compare May 4, 2026 10:18
@jfischer-no
Copy link
Copy Markdown
Contributor

What is the goal of this? OHCI is limited to full-speed, and is also used as a companion controller to EHCI, which is limited to high-speed. Though there are EHCI/OHCI shim drivers in the tree for the NXP controllers, I doubt that someone will spend time to get this driver working on real hardware. If that is about to get a controller supported in QEMU, then the right way would be to write XHCI driver. See also QEMU/USB emulation.

@tmon-nordic
Copy link
Copy Markdown
Contributor

tmon-nordic commented May 6, 2026

What is the goal of this? OHCI is limited to full-speed, and is also used as a companion controller to EHCI, which is limited to high-speed. Though there are EHCI/OHCI shim drivers in the tree for the NXP controllers, I doubt that someone will spend time to get this driver working on real hardware.

USB compliant High-Speed host port must support Low-Speed/Full-Speed/High-Speed devices connected to it. EHCI controller only supports High-Speed devices, you still need companion controller to handle Full-Speed/Low-Speed devices.

EHCI Revision 1.0 1. Introduction

[...]
Low-risk support for Full- and Low-speed peripherals. The EHCI specification provides support for all three device speeds on the root ports by integrating (using) existing hardware and software for USB 1.1 host controllers for support of Full- and Low-speed devices connected to the root ports. This allows the EHCI to focus on efficient support of high-speed operation, without having to accommodate any trade-offs in complexity or performance to support Full- and Low-speed devices.
[...]

If we are talking about modern x86 PCs, then maybe XHCI is the only thing that matters. But I highly doubt that all the embedded SOCs that do have EHCI/OHCI are end-of-life - which is what your comment seems to suggest.

@maass-hamburg
Copy link
Copy Markdown
Member Author

Mainly because of https://spinalhdl.github.io/SpinalDoc-RTD/master/SpinalHDL/Libraries/Com/usb_ohci.html

Because OHCI has a Open Source implementation for FPGAs.
And it just needs to regular pins of a FPGA, so no external phy. You only need some additional resistors.

That it works in qemu is a very nice bonus and that's how i tested it.

@jfischer-no
Copy link
Copy Markdown
Contributor

Mainly because of https://spinalhdl.github.io/SpinalDoc-RTD/master/SpinalHDL/Libraries/Com/usb_ohci.html

Because OHCI has a Open Source implementation for FPGAs. And it just needs to regular pins of a FPGA, so no external phy. You only need some additional resistors.

That makes much more sense.
With your generic OHCI driver, there will be two drivers for the OHCI. As there should not be any shim driver for this kind of controllers in the tree, at the time NXP's EHCI and OHCI drivers are introduced, we agreed that implementing a generic EHCI/OHCI driver would be too much work, assuming that it would be only one vendor platform using it. Meanwhile, there are more platforms with EHCI/OHCI, for example: sama7d6, sama7g5, sama5d2, sam9x7, stm32mp157axx, stm32mp251fxx. What should happen to NXP (EHCI) OHCI shim driver in the long term?

@maass-hamburg
Copy link
Copy Markdown
Member Author

What should happen to NXP (EHCI) OHCI shim driver in the long term?

In my opinion, it should be migrated to this driver. I have never been a big fan of using hals, as can be seen in #108088

Comment thread drivers/usb/uhc/Kconfig.ohci Outdated
Comment on lines +26 to +33
default 3
help
Number of concurrently active bulk/interrupt transfer slots. One slot
is always reserved for control transfers. Each additional slot allows
one more bulk or interrupt endpoint to stay posted simultaneously
(e.g. both IN and OUT for CDC-ECM data plus the interrupt notification
endpoint). Memory cost is one ED plus UHC_OHCI_MAX_TDS transfer
descriptors per slot.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the default increase to 4? 4 should allow CDC ECM (or CDC ACM, or any other class that uses 3 endpoints) to operate freely (one is always reserved for control transfers).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sure why not, imean we could even set it to 8 by default and leave it to the user to reduce it, if the target is that memory constraint

Comment on lines +80 to +81
#define OHCI_RHPS_W1C_MASK (OHCI_RHPS_CSC | OHCI_RHPS_PESC | OHCI_RHPS_PSSC | \
OHCI_RHPS_OCIC | OHCI_RHPS_PRSC)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is so special about these bits? CCS, PRS, PPS and LSDA are also W1C according to specification. Other defined fields which do not have corresponding #define statements here are also W1C.

Comment on lines +83 to +84
#define OHCI_ED_FA_SHIFT 0U
#define OHCI_ED_EN_SHIFT 7U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why no mask for FunctionAddress and EndpointNumber?

Comment on lines +85 to +88
#define OHCI_ED_D_SHIFT 11U
#define OHCI_ED_D_FROM_TD 0U
#define OHCI_ED_D_OUT 1U
#define OHCI_ED_D_IN 2U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why deviate from the style used with OHCI_CONTROL_HCFS_?

#define OHCI_ED_D_IN 2U
#define OHCI_ED_SPEED BIT(13)
#define OHCI_ED_SKIP BIT(14)
#define OHCI_ED_MPS_SHIFT 16U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why no mask for MaximumPacketSize?

#define OHCI_RHPS_W1C_MASK (OHCI_RHPS_CSC | OHCI_RHPS_PESC | OHCI_RHPS_PSSC | \
OHCI_RHPS_OCIC | OHCI_RHPS_PRSC)

#define OHCI_ED_FA_SHIFT 0U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would greatly appreciate /* 4.2.2 Endpoint Descriptor Field Definitions */ comment, as this is not so easy to find otherwise.

Comment on lines +98 to +110
#define OHCI_TD_CC_NO_ERROR 0U
#define OHCI_TD_CC_DATA_UNDERRUN 9U
#define OHCI_TD_CC_NOT_ACCESSED 15U
#define OHCI_TD_T_SHIFT 24U
#define OHCI_TD_T_TOGGLE_ED 0U
#define OHCI_TD_T_DATA0 2U
#define OHCI_TD_T_DATA1 3U
#define OHCI_TD_DI_SHIFT 21U
#define OHCI_TD_DI_NO_INTERRUPT 7U
#define OHCI_TD_DP_SHIFT 19U
#define OHCI_TD_DP_SETUP 0U
#define OHCI_TD_DP_OUT 1U
#define OHCI_TD_DP_IN 2U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why deviate from the style used with OHCI_CONTROL_HCFS_?

Why no mask for T, DI and DP?

Comment thread drivers/usb/uhc/uhc_ohci.c Outdated
Comment on lines +498 to +501
uint32_t ed_flags = FIELD_PREP(GENMASK(6, 0), xfer->udev->addr) |
FIELD_PREP(GENMASK(10, 7), USB_EP_GET_IDX(xfer->ep)) |
FIELD_PREP(GENMASK(31, 16), xfer->mps) |
FIELD_PREP(GENMASK(12, 11), OHCI_ED_D_FROM_TD);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the masks are generated inline here? Aren't there supposed to be defines for these?

Comment thread drivers/usb/uhc/uhc_ohci.c Outdated
Comment on lines +354 to +356
FIELD_PREP(GENMASK(25, 24), toggle) |
FIELD_PREP(GENMASK(20, 19), dp) |
FIELD_PREP(GENMASK(23, 21),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the masks are generated inline here? Aren't there supposed to be defines for these?

@@ -0,0 +1 @@
CONFIG_QEMU_EXTRA_FLAGS="-device usb-ccid"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using the smartcard reader because that is unlikely to get driver support in zephyr.

Why is it important that there is no driver support?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it isn't really, i just wanted to make sure to reduce problems in the future

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic May 8, 2026

Choose a reason for hiding this comment

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

In such case, I would rephrase the commit message to indicate that the usb-ccid is just arbitrary choice. Without going into the speculation whether it will get driver support or won't.

add Generic OHCI USB host controller driver support

Assisted-by: GitHub Copilot: GPT-5.3-Codex

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
We need to attach a usb device to the usb bus
for the test to work, therefore tell qemu to
add a usb smartcard reader. Using the smartcard
reader because that is unlikely to get driver support
in zephyr.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Add usb ohci host controller to the
qemu riscv32 board.

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
add qemu board config, that way a device
on the usb bus is being reported

Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
@maass-hamburg maass-hamburg force-pushed the ohci-generic-host-driver branch from bb131ca to 72b1a71 Compare May 7, 2026 16:33
@zephyrbot zephyrbot requested a review from tmon-nordic May 7, 2026 16:36
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-assisted At least one commit in the PR has an "Assisted-by: " entry.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants