drivers: usb: add Nordic USBHS wrapper and USBHS BC12 drivers by jfischer-no · Pull Request #106759 · zephyrproject-rtos/zephyr

jfischer-no · 2026-04-02T12:16:39Z

Add drivers for Nordic USBHS wrapper and USB PD Charging-Type detector.
Alternative to #105731

tmon-nordic

Significantly better than #105731 but there are errors in actual bit handling and the synchronization is currently only apparent. It should be possible to do proper synchronization with the overall structure.

tmon-nordic · 2026-04-03T08:36:53Z

+		/* Additionally suspend PHY */
+		inputoverride = USBHS_PHY_INPUTOVERRIDE_ID_Msk |
+				USBHS_PHY_INPUTOVERRIDE_VBUSVALID_Msk;


This path is not forcing PHY SUSPENDM0 to suspend mode, the code in if (suspended) does it.

tmon-nordic · 2026-04-03T08:41:35Z

+	struct nrf_usbhs_wrapper_data *const data = dev->data;
+
+	wrapper_enable_phy_internal(dev, suspended);
+	atomic_inc(&data->refcount);


How is this synchronization working? Reference counter is atomic, but the actual wrapper accesses are in no way protected. I think a spin lock would be necessary (can change the refcount to just normal variable then).

Synchronization between what? The UDC driver quirk does not access the wrapper until the BC12 driver finishes. The BC12 driver has preference over the UDC driver and always accesses wrapper registers from ISR context. UDC wrapper access seems to be doomed, but I could not observe it, perhaps because of the VREG hysteresis.

BC12 role can be changed independently of UDC state. Input overrides are effectively controlled by both UDC and BC12 driver.

wrapper_enable_phy_internal() is not in any way synchronized by the use of atomic_inc() at all.

tmon-nordic · 2026-04-03T08:42:46Z

+	/* Wait for PHY clock to start */
+	k_busy_wait(45);


Who controls reference clock? PHY clock won't start without it.

Yes, and it is still called from the UDC driver only.

tmon-nordic · 2026-04-03T08:45:59Z

+	const void *bc12_cb_data;
+	regulator_callback_t phy_cb;
+	const void *phy_cb_data;
+	atomic_t refcount;


I think there should be separate flag for whether udc and bc12 are enabled. Refcount is not really suitable here because the actual override sequence does depend on enabled order (bc12 or udc first) and vregusb interrupt.

No, I do not think so. Refcount is very well suited, otherwise it would not be there. It is also not about enabled order, when there is BC12 driver, it always takes the precedence.

UDC can be enabled and be kept enabled all the time. BC12 needs to perform Battery Charging operations on every VBUS detection (if role is set to Portable Device) and then leave with the state suitable for UDC to continue its operation.

tmon-nordic · 2026-04-03T08:49:22Z

+	const struct usbhs_bc_config *const config = dev->config;
+	NRF_USBHS_Type *const wrapper = config->base;
+
+	wrapper->PHY.OVERRIDEVALUES = USBHS_PHY_OVERRIDEVALUES_OPMODE0_Msk |


OPMODE0 has to be binary 01, not 11. BIT(USBHS_PHY_OVERRIDEVALUES_OPMODE0_Pos) will produce correct value.

tmon-nordic · 2026-04-03T08:50:05Z

+	NRF_USBHS_Type *const wrapper = config->base;
+
+	wrapper->PHY.OVERRIDEVALUES = USBHS_PHY_OVERRIDEVALUES_OPMODE0_Msk |
+				      USBHS_PHY_OVERRIDEVALUES_XCVRSEL0_Msk;


XCVRSEL0 has to be binary 01, not 11. BIT(USBHS_PHY_OVERRIDEVALUES_XCVRSEL0_Pos) will produce correct value.

tmon-nordic · 2026-04-03T08:56:19Z

+		/*
+		 * Enable VDP_SRC onto DP and keep it enabled until DWC2 driver
+		 * is ready and enables D+ pull-up.
+		 */
+		wrapper->PHY.BATTCHRG = USBHS_PHY_BATTCHRG_VDATSRCENB0_Msk;


This has to turned off in case DWC2 enables D+ pull-up.

Nordic USBHS wrapper is a set of registers used by both USB BC12 driver and vendor specific part in UDC driver. The wrapper can be considered to be MFD device. Both drivers depends on the VBUS regulator driver and require some synchronisation when accessing wrapper register. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

Use USBHS wrapper driver to register VREGUSB regulator callback and enable USB peripheral. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

Add driver for Nordic USB PD Charging-Type detector. This driver depends on the USBHS wrapper driver. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no> Co-authored-by: Tomasz Moń <tomasz.mon@nordicsemi.no>

Add and enable USBHS charging-type detector node. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

keith-zephyr

BC12 driver looks good to me

Add an example of how to use USB BC12 driver callback among the USB device support. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

sonarqubecloud · 2026-04-13T15:42:18Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tmon-nordic · 2026-04-23T11:24:17Z

-
-	wrapper->ENABLE = 0UL;
-
+	nrf_usbhs_wrapper_disable(parent);


Because this effectively cancels out nrf_usbhs_wrapper_enable_udc(), this should be renamed to nrf_usbhs_wrapper_disable_udc().

tmon-nordic · 2026-04-23T12:01:41Z

+{
+	const struct usbhs_bc_config *const config = dev->config;
+
+	nrf_usbhs_wrapper_disable(config->parent);


Because this effectively cancels out nrf_usbhs_wrapper_enable_phy(), this should be renamed to nrf_usbhs_wrapper_disable_phy() to match.

However, I think the nrf_usbhs_wrapper_enable_phy() should be changed to nrf_usbhs_wrapper_enable_bc() because what we need is BC functionality. Technically we cannot just enable PHY alone without enabling CORE. If nrf_usbhs_wrapper_enable_phy() gets renamed to nrf_usbhs_wrapper_enable_bc() then nrf_usbhs_wrapper_disable_bc() would be the correct name to use here.

tmon-nordic · 2026-04-23T12:09:49Z

+	if (role == BC12_DISCONNECTED) {
+		data->partner_state.type = BC12_TYPE_NONE;
+		data->partner_state.bc12_role = role;
+		err = nrf_usbhs_wrapper_vreg_disable(config->parent);
+		if (err) {
+			LOG_ERR("Failed to disable regulator");
+		}
+
+		return err;
+	}


The logic will still trigger BC operations if UDC driver called nrf_usbhs_wrapper_vreg_enable().

There are multiple ways to solve the problem. Example would be to check bc12_role before starting BC operations. The assigned role would need synchronization via spin lock because the check would be done inside interrupt handler. If role is set to BC12_DISCONNECTED while BC detection is in progress, then I believe the detection should still finish normally and the change should only refer to VBUS detections that happens after role was set.

tmon-nordic · 2026-04-23T12:20:59Z

+	struct nrf_usbhs_wrapper_data *const data = dev->data;
+
+	wrapper_enable_phy_internal(dev, suspended);
+	atomic_inc(&data->refcount);


BC12 role can be changed independently of UDC state. Input overrides are effectively controlled by both UDC and BC12 driver.

wrapper_enable_phy_internal() is not in any way synchronized by the use of atomic_inc() at all.

tmon-nordic · 2026-04-23T12:22:06Z

+	LOG_INF("Enable %s, refcount %lu",
+		"USB peripheral", atomic_get(&data->refcount));


This log is not atomic. The return value from atomic_inc() must be used instead of atomic_get() if you want to use the primitive correctly.

I believe just using the data->lock spinlock would be superior here. Using atomic API in non-atomic way is effectively making this rather pointless.

tmon-nordic · 2026-04-23T12:26:21Z

+	LOG_INF("BC12 detection finished, refcount %lu", atomic_get(&data->refcount));
+
+	k_event_post(&data->events, NRF_USBHS_PHY_READY);
+	if (atomic_get(&data->refcount) > 1) {


Using atomic here sounds like atomic misuse. There really should be a bit indicating that nrf_usbhs_wrapper_enable_udc() was called. The bit should be protected with a spin lock.

tmon-nordic · 2026-04-23T12:32:17Z

+	 */
+	wrapper->PHY.INPUTOVERRIDE = USBHS_PHY_INPUTOVERRIDE_ID_Msk;
+
+	if (IS_ENABLED(CONFIG_USB_BC12_NRF_USBHS)) {


This shouldn't be only based on whether the driver is compiled, but also if BC12 operations were performed.

tmon-nordic · 2026-04-23T12:47:25Z

+#include <nrf_usbhs_wrapper.h>
+
+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(usbhs, LOG_LEVEL_ERR);


It would be nice it if this was not hardcoded.

tmon-nordic · 2026-04-23T12:56:16Z

+	const void *bc12_cb_data;
+	regulator_callback_t phy_cb;
+	const void *phy_cb_data;
+	atomic_t refcount;


UDC can be enabled and be kept enabled all the time. BC12 needs to perform Battery Charging operations on every VBUS detection (if role is set to Portable Device) and then leave with the state suitable for UDC to continue its operation.

jfischer-no requested review from carlescufi, josuah, keith-zephyr and tmon-nordic April 2, 2026 12:16

zephyrbot added platform: nRF Nordic nRFx area: Boards/SoCs area: Samples Samples area: USB Universal Serial Bus area: Devicetree Binding PR modifies or adds a Device Tree binding labels Apr 2, 2026

zephyrbot requested review from JarmouniA, anangl, bjarki-andreasen, jaz1-nordic, kl-cruz, lstnl, magp-nordic, nashif, nika-nordic, nordic-krch and nordicjm April 2, 2026 12:18

zephyrbot assigned anangl, nordic-krch and bjarki-andreasen Apr 2, 2026

josuah mentioned this pull request Apr 2, 2026

usb: udc: dwc2: split vendor quirks per-vendor #105030

Merged

tmon-nordic requested changes Apr 3, 2026

View reviewed changes

josuah mentioned this pull request Apr 3, 2026

drivers: usb: uhc: refactored #104359

Closed

jfischer-no and others added 4 commits April 8, 2026 15:50

drivers: udc: use USBHS wrapper driver in nRF54LM20a vendor quirks

2d00d51

Use USBHS wrapper driver to register VREGUSB regulator callback and enable USB peripheral. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

drivers: usb: add Nordic USBHS BC12 driver

3dbfacb

Add driver for Nordic USB PD Charging-Type detector. This driver depends on the USBHS wrapper driver. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no> Co-authored-by: Tomasz Moń <tomasz.mon@nordicsemi.no>

board: nrf54lm20a: add USBHS charging-type detector node

e2d9348

Add and enable USBHS charging-type detector node. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no force-pushed the pr-nrf54lm20a-wrapper-bc12 branch 2 times, most recently from c28e199 to e6ee67b Compare April 9, 2026 16:23

keith-zephyr previously approved these changes Apr 9, 2026

View reviewed changes

samples: hid-mouse: modify sample to use USB BC12 drivers

c9d0a45

Add an example of how to use USB BC12 driver callback among the USB device support. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no dismissed keith-zephyr’s stale review via c9d0a45 April 13, 2026 15:20

jfischer-no force-pushed the pr-nrf54lm20a-wrapper-bc12 branch from e6ee67b to c9d0a45 Compare April 13, 2026 15:20

tmon-nordic requested changes Apr 23, 2026

View reviewed changes

		LOG_INF("Enable %s, refcount %lu",
		"USB peripheral", atomic_get(&data->refcount));

Conversation

jfischer-no commented Apr 2, 2026

Uh oh!

tmon-nordic 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

keith-zephyr left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Apr 13, 2026

Quality Gate passed

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants