drivers: usb: add Nordic USBHS BC12 driver #105731
drivers: usb: add Nordic USBHS BC12 driver #105731jfischer-no wants to merge 4 commits intozephyrproject-rtos:mainfrom
Conversation
jfischer-no
force-pushed
the
pr-nrf54lm20a-bc12
branch
from
ebaa49d to
63a26cc
Compare
zephyrbot
added
area: Regulators
area: USB
zephyrbot
requested review from
aasinclair,
anangl,
bjarki-andreasen,
danieldegrasse,
jaz1-nordic,
josuah,
kl-cruz,
lstnl,
magp-nordic,
nashif,
nika-nordic,
nordic-krch and
nordicjm
|
Needs rebase |
jfischer-no
force-pushed
the
pr-nrf54lm20a-bc12
branch
from
63a26cc to
538a164
Compare
jfischer-no
force-pushed
the
pr-nrf54lm20a-bc12
branch
from
538a164 to
d9fc3d9
Compare
| #define TVDPSRC_DIS 20 | ||
| #define TVDMSRC_DIS 20 | ||
|
|
||
|
|
| #include <zephyr/logging/log.h> | ||
| LOG_MODULE_REGISTER(main, LOG_LEVEL_INF); | ||
|
|
||
| static const struct device *bc12_dev = DEVICE_DT_GET_OR_NULL(nordic_nrf_usbhs_bc12); |
There was a problem hiding this comment.
Is this sample specific to nordic? otherwise consider changing this to a
zephyr,user {
sample-bc12-dev = <&usbhs_bc12>;
};
also, nordic_nrf_usbhs_bc12 is not a valid node name so this is always NULL from what I can see
jfischer-no
force-pushed
the
pr-nrf54lm20a-bc12
branch
from
d9fc3d9 to
cf539bb
Compare
Add driver for Nordic USB PD Charging-Type detector. 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>
Do not use VREGUSB driver directly when a USB BC12 driver is enabled. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no> Co-authored-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
Allow USB BC12 driver callback be used in hid-mouse sample. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
carlescufi
force-pushed
the
pr-nrf54lm20a-bc12
branch
from
cf539bb to
3c2a770
Compare
|
|
DNM pending internal discussion about the hardware. |
tmon-nordic
left a comment
tmon-nordic
left a comment
There was a problem hiding this comment.
I want to bring this to Architecture Review so we can have a clear guideline of what is generally expected in Zephyr project when it comes to what degree Kconfig options can require overall changes in applications.
| zephyr,flash-controller = &rram_controller; | ||
| zephyr,flash = &cpuapp_rram; | ||
| zephyr,ieee802154 = &ieee802154; | ||
| zephyr,usb-bc12 = &usbhs_bc12; |
There was a problem hiding this comment.
Undocumented zephyr,usb-bc12 chosen node added to facilitate extremely tightly coupling that is enforced onto applications that do enable USB_BC12_NRF_USBHS.
|
|
||
| if (CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT) { | ||
| timeout = K_MSEC(CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT); | ||
| } | ||
| if (!IS_ENABLED(CONFIG_USB_BC12_NRF_USBHS)) { | ||
| k_timeout_t timeout = K_FOREVER; | ||
|
|
||
| if (!k_event_wait(&usbhs_events, USBHS_VBUS_READY, false, K_NO_WAIT)) { | ||
| LOG_WRN("VBUS is not ready, block udc_enable()"); | ||
| if (!k_event_wait(&usbhs_events, USBHS_VBUS_READY, false, timeout)) { | ||
| return -ETIMEDOUT; | ||
| if (CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT) { | ||
| timeout = K_MSEC(CONFIG_UDC_DWC2_USBHS_VBUS_READY_TIMEOUT); | ||
| } | ||
|
|
||
| if (!k_event_wait(&usbhs_events, USBHS_VBUS_READY, false, K_NO_WAIT)) { | ||
| LOG_WRN("VBUS is not ready, block udc_enable()"); | ||
| if (!k_event_wait(&usbhs_events, USBHS_VBUS_READY, false, timeout)) { | ||
| return -ETIMEDOUT; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Having IS_ENABLED(CONFIG_USB_BC12_NRF_USBHS)) in here is probably the best argument behind that Nordic DWC2 vendor quirks have to be synchronized against Nordic BC1.2 driver.
Instead of proper synchronization to a shared resource (USBHS wrapper registers), the IS_ENABLED(CONFIG_USB_BC12_NRF_USBHS) checks in DWC2 vendor quirk effectively remove any guards around the shared resources, which is in my opinion just wrong.
This design choice effectively forces tightly coupling on application. If application does not adhere to the implicitly imposed tightly coupling, then things will break very badly.
If this PR is merged at its current state, then west build -p -b nrf54lm20dk/nrf54lm20b/cpuapp -d build-testusb-54lm20-with-bc samples/subsys/usb/testusb/ -- -DCONFIG_USB_BC12=y will produce application that does not work at all. Even worse, debugger will hang making it very hard for the developer to determine even where the problem is (if there was a Bus Fault or Hard Fault it would be a trivial, but there isn't any such clue).
Without CONFIG_USB_BC12=y the same code built using west build -p -b nrf54lm20dk/nrf54lm20b/cpuapp -d build-testusb-54lm20 samples/subsys/usb/testusb/ produces perfectly working application.
The "solution" (to the root cause behind the hang) presented here is to enforce tightly coupling in application code so that it only enables USB stack after BC12 detection finishes and so the application disables USB stack when VBUS is gone. The two major drawbacks of this approach are:
- user is IMHO unreasonably expected to know that such tightly coupling is necessary
- application cannot choose to not perform BC12 detection (i.e. not configure BC12 role as Portable Device) if Nordic BC12 driver Kconfig is enabled
I would like to have it answered at Architecture level, whether such tightly coupling is appropriate for Zephyr or not. In other words, is it perfectly valid to have applications break completely if a single Kconfig option is enabled (here: CONFIG_USB_BC12=y)?
| if (IS_ENABLED(CONFIG_USB_BC12)) { | ||
| if (!device_is_ready(bc12_dev)) { | ||
| LOG_ERR("USB BC1.2 device %s is not ready", bc12_dev->name); | ||
| return -EIO; | ||
| } | ||
|
|
||
| k_work_init(&app_bc_priv.work, app_bc12_work_handler); | ||
|
|
||
| bc12_set_result_cb(bc12_dev, &bc12_result_cb, sample_usbd); | ||
| ret = bc12_set_role(bc12_dev, BC12_PORTABLE_DEVICE); | ||
| if (ret != 0) { | ||
| LOG_ERR("Failed to set BC12 role"); | ||
| return -EIO; | ||
| } | ||
| } else { | ||
| ret = usbd_enable(sample_usbd); | ||
| if (ret != 0) { | ||
| LOG_ERR("Failed to enable device support"); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
I wouldn't mind this if it wasn't the absolutely only working code. That is, as an example use case, this is fine. But here the reason behind these changes is to effectively workaround complete lack of synchronization between DWC2 vendor quirk and Nordic BC1.2 driver.
tmon-nordic
added
the
Architecture Review
carlescufi
removed
the
Architecture Review
12 participants
Add driver for Nordic USB PD Charging-Type detector.
Depends on: #97642