Skip to content

drivers: udc_dwc2: Add nRF54L20 UDC DWC2 quirks #80384

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

drivers: udc_dwc2: Add nRF54L20 UDC DWC2 quirks #80384

wants to merge 2 commits into from

Conversation

@e-rk
Copy link
Contributor

@e-rk e-rk commented Oct 24, 2024 •
edited
Loading

PR consists of 3 changes:

  • Add option to select 8-bit UTMI+ PHY interface
  • Add DTS nodes to nRF54L20
  • Add quirks for nRF54L20

@zephyrbot zephyrbot added the platform: nRF Nordic nRFx label Oct 24, 2024
@zephyrbot zephyrbot added the area: USB Universal Serial Bus label Oct 24, 2024
jfischer-no
jfischer-no requested changes Oct 24, 2024
View reviewed changes
drivers/usb/udc/udc_dwc2_vendor_quirks.h Outdated
#endif /*DT_HAS_COMPAT_STATUS_OKAY(st_stm32f4_fsotg) */

#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_usbhs)
#if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_usbhs_nrfs)
Copy link
Contributor

@jfischer-no jfischer-no Oct 24, 2024

Choose a reason for hiding this comment

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

There is no point in renaming it or postfixing it with -nrfs. And there is no reason to break someone else's work with a compatible rename. You will need to communicate with us internally to find a suitable compatible.

Copy link
Contributor

@tmon-nordic tmon-nordic Oct 24, 2024

Choose a reason for hiding this comment

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

There is a direct need for solving the compatible naming issue. Both nRF54H20 and nRF54L20 do have USBHS peripheral. Yet, the nRF54H20 requires nrfs to get information about VBUS and nRF54L20 has USBREG peripheral for that. Therefore I think the nordic,nrf-usbhs-nrfs and nordic,nrf-usbhs-usbreg is probably the cleanest approach.

Copy link
Contributor Author

@e-rk e-rk Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have no strong opinion, although I think the -nrfs suffix makes the difference clear. I'll leave it up to your preference, just please confirm that this is what you want after reading Tomasz's comment.

Copy link
Contributor

@jfischer-no jfischer-no Oct 25, 2024

Choose a reason for hiding this comment

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

I am not convinced to approve changes that break other people's work for no reason. "nordic, nrf-usbhs" should not be renamed. Also, new compatilbe needs a better name, something like "nordic, nrf-usbhs-fancy-soc-family". (nrfs is some software, usbreg is ambiguous)

Copy link
Contributor Author

@e-rk e-rk Oct 25, 2024

Choose a reason for hiding this comment

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

I have removed the nordic,nrf-usbhs change and the nRF54L20 now has nordic,nrf-usbhs-nrf54l compatible instead.

drivers/usb/udc/Kconfig.dwc2 Outdated
Comment on lines 51 to 58
Copy link
Contributor

@jfischer-no jfischer-no Oct 24, 2024

Choose a reason for hiding this comment

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

I can not see any justification why 8-bit should be preferred. And Kconfig is just the wrong place to configure it.

Copy link
Contributor

@tmon-nordic tmon-nordic Oct 24, 2024

Choose a reason for hiding this comment

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

If there is UTMI+ PHY capable of both 8-bit and 16-bit operation connected to DWC2 controller then there is very little need to use 8-bit (clocking is different between the two, so there hardware behavior changes slightly).

However, it is also possible that there is only 8-bit capable UTMI+ PHY connected to DWC2 controller that supports both 8-bit and 16-bit. In such case this is a killer feature (because 16-bit simply won't work).

drivers/usb/udc/udc_dwc2_vendor_quirks.h Outdated
Comment on lines 336 to 386
Copy link
Contributor

@jfischer-no jfischer-no Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The existing can be move within #if DT_HAS_COMPAT_STATUS_OKAY(x) || DT_HAS_COMPAT_STATUS_OKAY(y) ... #endif and reused.

Copy link
Contributor

@tmon-nordic tmon-nordic Oct 24, 2024

Choose a reason for hiding this comment

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

I think you meant || instead of &&.

Copy link
Contributor Author

@e-rk e-rk Oct 25, 2024

Choose a reason for hiding this comment

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

I have moved the common parts to a common #if

@e-rk e-rk force-pushed the usbhs-upstream branch 3 times, most recently from f00d54a to 09fe93b Compare October 25, 2024 10:52
@e-rk e-rk requested a review from jfischer-no November 8, 2024 15:07
@e-rk
Copy link
Contributor Author

e-rk commented Nov 19, 2024

@tmon-nordic @jfischer-no could you take a look?

tmon-nordic
tmon-nordic reviewed Nov 19, 2024
View reviewed changes
drivers/usb/udc/udc_dwc2.c Outdated
Copy link
Contributor

@tmon-nordic tmon-nordic Nov 19, 2024

Choose a reason for hiding this comment

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

I think LOG_DBG("Using 8-bit UTMI+"); would be more appropriate here.

Copy link
Contributor Author

@e-rk e-rk Nov 19, 2024

Choose a reason for hiding this comment

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

Done

@e-rk e-rk force-pushed the usbhs-upstream branch 2 times, most recently from 901e981 to 9a34724 Compare November 25, 2024 13:41
masz-nordic
masz-nordic previously approved these changes Nov 27, 2024
View reviewed changes
@e-rk
Copy link
Contributor Author

e-rk commented Nov 28, 2024

It looks like the tests are consistently failing in

 141/697 nrf54l20pdk/nrf54l20/cpuapp drivers.uart.async_api.rtt                         ERROR Build failure (build)

due to

drivers__serial.dir/uart_nrfx_uarte.c.obj -c /__w/zephyr/zephyr/drivers/serial/uart_nrfx_uarte.c
In file included from /__w/zephyr/zephyr/drivers/serial/uart_nrfx_uarte.c:15:
In function 'nrf_uarte_configure',
    inlined from 'uarte_nrfx_configure' at /__w/zephyr/zephyr/drivers/serial/uart_nrfx_uarte.c:504:2:
/__w/zephyr/modules/hal/nordic/nrfx/hal/nrf_uarte.h:877:23: error: 'uarte_cfg.frame_timeout' may be used uninitialized [-Werror=maybe-uninitialized]
  877 |                     | (uint32_t)p_cfg->frame_timeout
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/zephyr/zephyr/drivers/serial/uart_nrfx_uarte.c: In function 'uarte_nrfx_configure':
/__w/zephyr/zephyr/drivers/serial/uart_nrfx_uarte.c:443:28: note: 'uarte_cfg.frame_timeout' was declared here
  443 |         nrf_uarte_config_t uarte_cfg;
      |                            ^~~~~~~~~
cc1: all warnings being treated as errors

which is weird, because I haven't done anything with UARTE. Has adding a new node to DTS enabled some new tests in twister?

@e-rk
Copy link
Contributor Author

e-rk commented Jan 16, 2025

CI is green. @tmon-nordic @jfischer-no @masz-nordic could you review?

kl-cruz
kl-cruz previously approved these changes Jan 29, 2025
View reviewed changes
@e-rk
Copy link
Contributor Author

e-rk commented Feb 19, 2025

CI is green. @tmon-nordic @jfischer-no @masz-nordic could you review?

Ping

masz-nordic
masz-nordic previously approved these changes Feb 21, 2025
View reviewed changes
@carlescufi carlescufi requested a review from tmon-nordic March 20, 2025 13:21
e-rk added 2 commits March 20, 2025 16:51
@e-rk
dts: common: nordic: nrf54l20: add USBHS node
f10cefb 
Added USBHS device tree node for nRF54L20.

Signed-off-by: Rafał Kuźnia <[email protected]>
@e-rk
drivers: udc_dwc2: Add nRF54L20 UDC DWC2 quirks
b056f38 
The nRF54L20 does not have the NRFS layer and the set of quirks will be
different than in nRF54H20. Added new set of quirks for the nRF54L20
SoC.

Signed-off-by: Rafał Kuźnia <[email protected]>
@e-rk e-rk dismissed stale reviews from masz-nordic and kl-cruz via b056f38 March 20, 2025 15:52
@e-rk
Copy link
Contributor Author

e-rk commented Mar 20, 2025

I have removed the commit that introduced CONFIG_UDC_DWC2_PREFER_8BIT_UTMI.

@github-actions
Copy link

github-actions bot commented May 20, 2025

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 20, 2025
@tmon-nordic
Copy link
Contributor

tmon-nordic commented May 20, 2025

Initial quirks implementation with the changes necessary to work on actual silicon is downstream at nrfconnect/sdk-zephyr#2770

Support will be upstreamed together with rest of nRF54L20 support.

@e-rk e-rk deleted the usbhs-upstream branch May 29, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@masz-nordic masz-nordic masz-nordic approved these changes

@kl-cruz kl-cruz kl-cruz left review comments

@anangl anangl Awaiting requested review from anangl

@jaz1-nordic jaz1-nordic Awaiting requested review from jaz1-nordic

@magp-nordic magp-nordic Awaiting requested review from magp-nordic

@nika-nordic nika-nordic Awaiting requested review from nika-nordic

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@tmon-nordic tmon-nordic Awaiting requested review from tmon-nordic

Assignees

@jfischer-no jfischer-no

Labels

area: USB Universal Serial Bus platform: nRF Nordic nRFx Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@e-rk @tmon-nordic @kl-cruz @jfischer-no @masz-nordic @zephyrbot