usb: device_next: Add Kconfig to set maximum speed #76255

tmon-nordic · 2024-07-24T07:49:46Z

Setting maximum supported speed to Full-Speed can reduce code size by allowing compiler optimizations at compile time and also helps reducing RAM by allocating smaller bulk buffers. More RAM optimizations are possible but this commit only gets the low hanging fruits.

Another idea is that High-Speed capable drivers can use this information to limit operating speed to user choice. This PR implements this only for udc_dwc2 driver.

Implements: #75002

henrikbrixandersen · 2024-07-24T08:16:02Z

subsys/usb/device_next/Kconfig

+config USBD_MAX_SPEED
+	int
+	default 0 if USBD_MAX_SPEED_FULL
+	default 1 if USBD_MAX_SPEED_HIGH


How come this indirection is needed? Why not simple do if (IS_ENABLED(CONFIG_USBD_MAX_SPEED_HIGH) in the C code?

The choice here makes it possible to add Super-Speed support later on without much hassle. IS_ENABLED(CONFIG_USBD_MAX_SPEED_HIGH) would make things harder IMHO.

That makes sense, thanks. I'll take a closer look later today.

jfischer-no · 2024-07-24T14:36:55Z

drivers/usb/udc/udc_dwc2.c

+		dcfg |= usb_dwc2_set_dcfg_devspd(USBD_SUPPORTS_HIGH_SPEED ?
+		    USB_DWC2_DCFG_DEVSPD_USBHS20 : USB_DWC2_DCFG_DEVSPD_USBFS20);
 		break;
 	case USB_DWC2_GHWCFG2_HSPHYTYPE_UTMIPLUS:
 		gusbcfg |= USB_DWC2_GUSBCFG_PHYSEL_USB20 |
 			   USB_DWC2_GUSBCFG_ULPI_UTMI_SEL_UTMI;
-		dcfg |= USB_DWC2_DCFG_DEVSPD_USBHS20
-			<< USB_DWC2_DCFG_DEVSPD_POS;
+		dcfg |= usb_dwc2_set_dcfg_devspd(USBD_SUPPORTS_HIGH_SPEED ?
+		    USB_DWC2_DCFG_DEVSPD_USBHS20 : USB_DWC2_DCFG_DEVSPD_USBFS20);


This layer violation. You may not use anything from subsys/usb/device_next. And there is absolutely no need to limit the high-speed controller to full speed.

There are valid reasons for disabling high-speed, otherwise the peripherals wouldn't support it.

Setting the reasons aside (the most trivial one is replicating an existing device with a newer SoC in a way that allows reusing existing host software without any modifications), please guide how to achieve this functionality without "layer violation". Note that USB itself has design choices that could be classified as layering violations, like:

different configurations based on operating speed

the meaning of bInterval field in Endpoint descriptor depends on operating speed

isochronous feedback size and meaning depends on operating speed

The operating speed plays so significant role in USB that I believe there must be a way to force Full-Speed only operation of High-Speed capable device without resorting to filtering out high-speed detection handshake on D+/D- lines.

jfischer-no · 2024-07-24T14:43:39Z

subsys/usb/device_next/Kconfig

+choice USBD_MAX_SPEED_CHOICE
+	prompt "Max supported connection speed"
+	default USBD_MAX_SPEED_FULL if UDC_IT82XX2 || UDC_KINETIS || UDC_NRF || UDC_STM32
+	default USBD_MAX_SPEED_HIGH


This layer violation. You may not use anything from drivers/usb//Kconfig. or have any dependencies on drivers.

Then how do you enable the easy and quite significant code and RAM savings on platforms that have controllers that are not capable of high-speed operation? Please come up with alternative instead of just screaming "layer violation".

Let's consider CDC ACM sample to be representative one. When building it for nRF52840DK (CONF_FILE=usbd_next_prj.conf west build --pristine -b nrf52840dk/nrf52840 -d build-cdc-acm-next ~/zephyrproject/zephyr/samples/subsys/usb/cdc_acm) on 596ef0a the summary is:

Memory region Used Size Region Size %age Used FLASH: 61636 B 1 MB 5.88% RAM: 20976 B 256 KB 8.00% IDT_LIST: 0 GB 32 KB 0.00%

With this PR the summary is:

Memory region Used Size Region Size %age Used FLASH: 60924 B 1 MB 5.81% RAM: 20080 B 256 KB 7.66% IDT_LIST: 0 GB 32 KB 0.00%

Note that this PR does only the low-hanging fruits and more savings would be possible (but would require a lot of COND_CODE which may be too costly maintenance-wise).

subsys/usb/device_next/Kconfig

bjarki-andreasen

The speed choice looks good now :)

tmon-nordic · 2024-08-01T09:22:12Z

Rebased to fix merge conflicts.

tmon-nordic · 2024-08-28T04:32:56Z

Rebased to fix merge conflicts.

tmon-nordic · 2025-03-05T07:49:41Z

This PR achieves two main goals:

Reduce code and RAM usage on Full-Speed only targets
Allow developer to limit the maximum operating speed

The design that enables above goals boils down to:

Every driver that supports High-Speed selects UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT
There is one and only one user-configurable Kconfig USBD_MAX_SPEED_CHOICE
- the choice defaults to maximum supported speed (High-Speed if selected)
- High-Speed choice is only available when UDC_DRIVER_HAS_HIGH_SPEED_SUPPORT is selected
- user can always lower the speed (USB mandates that every High-Speed device must allow Full-Speed operation)
- If and only if the stack is configured to support High-Speed, the hidden Kconfig UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED is selected
When Full-Speed is the selected maximum speed, then High-Speed support is gone from stack (at compile time)
- USB classes use if (USBD_SUPPORTS_HIGH_SPEED) construct to wrap High-Speed only functionality
High-Speed capable drivers, configure peripheral to limit maximum operating speed based on hidden Kconfig UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED

Architecture Working Group is expected to resolve following disagreement:

a) Whether allowing user to limit High-Speed capable controller to operate at Full-Speed only is supported use case.

@tmon-nordic strongly believes this is normal use case, because USB controller is just one of many features that are determining the SoC selection. Every High-Speed capable controller can operate at Full-Speed (otherwise it wouldn't be certified), therefore the user can search for SoCs with Full-Speed or High-Speed capable controllers if the end application will only ever operate at Full-Speed.

b) Whether having a single user-configurable source of truth for maximum supported speed (Kconfig choice USBD_MAX_SPEED_CHOICE) is appropriate

Future extensibility considerations:

Super-Speed devices with USB connector must support at least one USB 2.x speed (Low/Full/High)
- currently USB stack does not support Low speed at all. Low-Speed support can be added on base of this PR later on without affecting overall user experience.
- currently Full-Speed is always enabled
- the Super-Speed devices could have separate choice entry for:
  Super-Speed + High-Speed (Full-Speed support is implied by USB specification)
  Super-Speed + Full-Speed
  Super-Speed + Low-Speed (when stack supports Low-Speed)
The additional choices (entries in Kconfig selection box) could be used to determine:
- Macros values: USBD_SUPPORTS_HIGH_SPEED, USBD_SUPPORTS_SUPER_SPEED (and possibly USBD_SUPPORTS_FULL_SPEED and USBD_SUPPORTS_LOW_SPEED)
- UDC hidden Kconfig symbols: UDC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED, UDC_DRIVER_SUPER_SPEED_SUPPORT_ENABLED (and possibly UDC_DRIVER_FULL_SPEED_SUPPORT_ENABLED, UDC_DRIVER_LOW_SPEED_SUPPORT_ENABLED)

henrikbrixandersen · 2025-03-05T08:08:12Z

The Option 3 is now implemented in this PR.

For completeness (and to avoid confusion), could we have option 3 added to the table in #76255 (comment) (or create a new table with all three options listed)?

carlescufi · 2025-03-11T13:09:25Z

A quick note from me as part of the upcoming discussion:

@jfischer-no wrote:

In general, I am confident that it should not be possible to limit the speed of the controller. There is no benefit to doing so in the real life of a USB device, and we should not bloat Kconfig with unnecessary options

Not only there is a tangible benefit, but Nordic has specific requests from customers to implement this. The reasoning is simple, as outlined by the customer: smaller code size, reduced complexity, ability to reuse parts of verification/internal certification.

jfischer-no · 2025-03-11T14:59:13Z

In general, I am confident that it should not be possible to limit the speed of the controller. There is no benefit to doing so in the real life of a USB device, and we should not bloat Kconfig with unnecessary options

Not only there is a tangible benefit, but Nordic has specific requests from customers to implement this. The reasoning is simple, as outlined by the customer: smaller code size, reduced complexity, ability to reuse parts of verification/internal certification.

This would only reduce memory usage. It does not reduce code size, the code is already there and is always built. This PR actually increases complexity. Again, for a USB device, if you replace a full-speed device with another that is a high-speed device, then no verification/certification can be reused because it is a new device regardless of speed.

henrikbrixandersen · 2025-03-11T15:04:13Z

Again, for a USB device, if you replace a full-speed device with another that is a high-speed device, then no verification/certification can be reused because it is a new device regardless of speed.

That's the exact reason for a customer wanting to limit the maximum USB speed.

jfischer-no · 2025-03-11T15:12:04Z

Again, for a USB device, if you replace a full-speed device with another that is a high-speed device, then no verification/certification can be reused because it is a new device regardless of speed.

That's the exact reason for a customer wanting to limit the maximum USB speed.

I do not know the customer's exact reason. I would be surprised if you did. However, the customer will need to re-certify/verify the new USB device because it would be a completely new device, regardless of what maximum speed the final product will support.

cfriedt · 2025-03-11T16:03:50Z

I would like to ask, why not just depend on devicetree for this in option 2, though? The user could set the maximum supported speed for their application/board per USB device controller and have the driver Kconfig pick this up?

devicetree is to mostly to describe hardware capabilities. While Zephyr does use it more for configuration, I think the UDC nodes should really describe what the hardware is capable of.

That was the question that immediately jumped out to me as well.

decsny · 2025-03-11T16:10:56Z

@jfischer-no is there some reason that the driver layer cant have a choice-type config structure to get the "ease of use" like in option 1/3 but not depend on upper layer?

decsny · 2025-03-11T16:12:19Z

I would like to ask, why not just depend on devicetree for this in option 2, though? The user could set the maximum supported speed for their application/board per USB device controller and have the driver Kconfig pick this up?

devicetree is to mostly to describe hardware capabilities. While Zephyr does use it more for configuration, I think the UDC nodes should really describe what the hardware is capable of.

That was the question that immediately jumped out to me as well.

yet another case of zephyr missing a multi instance static software config system

jfischer-no · 2025-03-11T16:23:04Z

@jfischer-no is there some reason that the driver layer cant have a choice-type config structure to get the "ease of use" like in option 1/3 but not depend on upper layer?

See #76255 (comment)
Also, option 3 does not provide the solution to problem "allowing user to limit High-Speed capable controller to operate at Full-Speed only is supported use case" and "having a single user-configurable source of truth for maximum supported speed". Only option 2 is consistently correct, because if you want to limit the controller, you should do it at the controller driver level, and the controller driver already is and will remain the "single source of truth for maximum supported speed".

jfischer-no · 2025-03-11T16:26:27Z

I would like to ask, why not just depend on devicetree for this in option 2, though? The user could set the maximum supported speed for their application/board per USB device controller and have the driver Kconfig pick this up?

devicetree is to mostly to describe hardware capabilities. While Zephyr does use it more for configuration, I think the UDC nodes should really describe what the hardware is capable of.

That was the question that immediately jumped out to me as well.

yet another case of zephyr missing a multi instance static software config system

This PR has nothing to do with DT or multi-instance static software configuration system. There is no dependency on DT in the USB device stack next.

tmon-nordic · 2025-03-11T16:44:58Z

Also, option 3 does not provide the solution to problem "allowing user to limit High-Speed capable controller to operate at Full-Speed only is supported use case" and "having a single user-configurable source of truth for maximum supported speed".

I disagree. Option 1/3 is precisely what has the one and only one Kconfig option to limit the application as a whole to Full-Speed only.

Only option 2 is consistently correct, because if you want to limit the controller, you should do it at the controller driver level, and the controller driver already is and will remain the "single source of truth for maximum supported speed".

You don't seem to accept that the real world use case for the user is to limit the USB stack (external device behavior) to particular speed. Forcing the controller to disable High-Speed chirp is just a necessary consequence of the user choice to limit the stack.

decsny · 2025-03-11T17:30:26Z

I would like to ask, why not just depend on devicetree for this in option 2, though? The user could set the maximum supported speed for their application/board per USB device controller and have the driver Kconfig pick this up?

devicetree is to mostly to describe hardware capabilities. While Zephyr does use it more for configuration, I think the UDC nodes should really describe what the hardware is capable of.

That was the question that immediately jumped out to me as well.

yet another case of zephyr missing a multi instance static software config system

This PR has nothing to do with DT or multi-instance static software configuration system. There is no dependency on DT in the USB device stack next.

yes I know, but someone else brought it up, and I think you can't deny it would be nice to be able configure the limit per device since many platforms have multiple devices (not endpoints, devices) that use the same driver. But it is outside of the scope of this PR so we don't need to discuss about it here, you're right

@jfischer-no is there some reason that the driver layer cant have a choice-type config structure to get the "ease of use" like in option 1/3 but not depend on upper layer?

See #76255 (comment) Also, option 3 does not provide the solution to problem "allowing user to limit High-Speed capable controller to operate at Full-Speed only is supported use case" and "having a single user-configurable source of truth for maximum supported speed". Only option 2 is consistently correct, because if you want to limit the controller, you should do it at the controller driver level, and the controller driver already is and will remain the "single source of truth for maximum supported speed".

I read your comment and you said

In general, I am confident that it should not be possible to limit the speed of the controller. There is no benefit to doing so in the real life of a USB device, and we should not bloat Kconfig with unnecessary options.

I think we are past this by now in the discussion because it was made clear that the goal is to configure a device to act only as full speed for the purpose of footprint savings, whether you personally want to do this is besides the point. And as far as Kconfig bloat, I don't think that is even valid here. I am willing to completely agree with you on this statement you made:

there must be no dependency in the driver on the higher level, nor should higher level change anything in the driver other than to enable it. This is fundamental, with no exceptions.

because to me that seems like the most sound architecture, and therefore your option 2 seems preferable. But I also have to say that at first looking at the comparison comment above , at a glance, the option 1/3 looks more preferable from a user perspective. Whether it is long term a good design is more important of course, but what I don't understand is why would you be opposed to a solution that combines your architectural awareness with the usability of the other two options. It seems obvious to me here unless I don't understand something that we can get the best of both worlds simply by introducing the user choice config similar to what is option 1/3 at the driver level to be fundamentally like option 2. What is the complication here, if there is any, from a technical (non opinionated) perspective? and don't say "bloat" or "unnecessary"

tmon-nordic · 2025-03-12T14:33:38Z

it would be nice to be able configure the limit per device since many platforms have multiple devices (not endpoints, devices) that use the same driver. But it is outside of the scope of this PR so we don't need to discuss about it here, you're right

If the user chooses to limit USB functionality (to Full-Speed only) at the stack level, then it doesn't matter what underlying drivers are there (all drivers should be aware that they are operating with Full-Speed only stack and therefore should disable High-Speed chirp).

It seems obvious to me here unless I don't understand something that we can get the best of both worlds simply by introducing the user choice config similar to what is option 1/3 at the driver level to be fundamentally like option 2. What is the complication here, if there is any, from a technical (non opinionated) perspective? and don't say "bloat" or "unnecessary"

The technical point is that by having the user-selectable option in UDC to limit the speed, then we would have to either:

Ditch the USB stack limiting option completely, and base the stack limits based on UDC Kconfig option. OR
Allow the "Kconfig trap" that makes it possible to build "full-speed only driver with high-speed capable stack".

Whether it is long term a good design is more important of course, but what I don't understand is why would you be opposed to a solution that combines your architectural awareness with the usability of the other two options

This is what option 3 is really. There is a hidden UDC Kconfig selected based on stack functionality so the UDC drivers can disable High-Speed chirp based on a UDC Kconfig instead of USB stack configuration.

tmon-nordic · 2025-04-07T06:34:55Z

This whole disagreement is summed up in the latest commit "usb: device_next: Change speed selection Kconfig dependency". I believe this commit is extremely harmful, but it may be the only way to move this forward.

Fix a typo resulted in USBD_SPEED_HS unregistering Full-Speed class instead of High-Speed. Signed-off-by: Tomasz Moń <[email protected]>

Two main ideas behind setting maximum speed are: * Allow code and RAM optimizations at compile time * Allow High-Speed capable drivers to limit operating speed to user choice. This commit only introduces the necessary Kconfig options but does not implement any code or RAM optimizations and does not modify any driver. Signed-off-by: Tomasz Moń <[email protected]>

Allow compiler optimizations to remove High-Speed handling code. Knowing that maximum operating speed is Full-Speed allows to reduce bulk buffers from 512 to 64 bytes. More RAM optimizations are possible but this commit only gets the low hanging fruits. Signed-off-by: Tomasz Moń <[email protected]>

Limit maximum operating speed in DCFG register if USB stack is configured to support only Full-Speed. Signed-off-by: Tomasz Moń <[email protected]>

In my opinion, the user is supposed to configure the speed of the stack and drivers ough to honor that choice. However current Zephyr USB maintainer imposes that the dependency is the other way round, i.e. that user first needs to disable High-Speed chirp at driver level and only then can select Full-Speed only operation. Adhere to the arbitrarily set up rule to allow this really necessary functionality to enter Zephyr. I consider this change to be harmful because it opens up a Kconfig trap that allows configuring High-Speed capable stack with a device driver limited to Full-Speed only operation. Signed-off-by: Tomasz Moń <[email protected]>

tmon-nordic requested review from henrikbrixandersen and jfischer-no July 24, 2024 07:49

zephyrbot added area: USB Universal Serial Bus area: Samples Samples labels Jul 24, 2024

zephyrbot requested review from kartben and nashif July 24, 2024 07:50

zephyrbot assigned jfischer-no Jul 24, 2024

henrikbrixandersen reviewed Jul 24, 2024

View reviewed changes

jfischer-no requested changes Jul 24, 2024

View reviewed changes

tmon-nordic force-pushed the max-speed branch from d7b5cf3 to 584bbaa Compare July 31, 2024 12:07

zephyrbot added the platform: NXP Drivers NXP Semiconductors, drivers label Jul 31, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, dleach02, manuargue and mmahadevan108 July 31, 2024 12:07

tmon-nordic force-pushed the max-speed branch 2 times, most recently from e14c6eb to 8907371 Compare July 31, 2024 12:59

bjarki-andreasen self-requested a review July 31, 2024 13:26

decsny removed their request for review July 31, 2024 18:20

jfischer-no reviewed Jul 31, 2024

View reviewed changes

subsys/usb/device_next/Kconfig Show resolved Hide resolved

tmon-nordic force-pushed the max-speed branch from 8907371 to 8017c5f Compare August 1, 2024 06:32

bjarki-andreasen previously approved these changes Aug 1, 2024

View reviewed changes

tmon-nordic dismissed bjarki-andreasen’s stale review via b3019a9 August 1, 2024 09:21

tmon-nordic force-pushed the max-speed branch from 8017c5f to b3019a9 Compare August 1, 2024 09:21

tmon-nordic force-pushed the max-speed branch from b3019a9 to cbaf2cb Compare August 1, 2024 13:17

tmon-nordic force-pushed the max-speed branch from cbaf2cb to 0a30a6e Compare August 28, 2024 04:32

github-project-automation bot moved this to Todo in Architecture Review Mar 4, 2025

carlescufi moved this from Todo to In Progress in Architecture Review Mar 17, 2025

tmon-nordic added 4 commits April 7, 2025 08:49

usb: device_next: Unregister class according to speed

c377d24

Fix a typo resulted in USBD_SPEED_HS unregistering Full-Speed class instead of High-Speed. Signed-off-by: Tomasz Moń <[email protected]>

drivers: udc_dwc2: Limit operating speed to stack configuration

0f771a4

Limit maximum operating speed in DCFG register if USB stack is configured to support only Full-Speed. Signed-off-by: Tomasz Moń <[email protected]>

tmon-nordic force-pushed the max-speed branch from ef0317f to 8113d80 Compare April 7, 2025 06:51

carlescufi removed the Architecture Review Discussion in the Architecture WG required label Apr 7, 2025

tmon-nordic force-pushed the max-speed branch from 8113d80 to c9100dc Compare April 7, 2025 12:45

jfischer-no approved these changes Apr 10, 2025

View reviewed changes

carlescufi approved these changes Apr 10, 2025

View reviewed changes

kartben merged commit d6a8bd5 into zephyrproject-rtos:main Apr 10, 2025
24 checks passed

github-project-automation bot moved this from In Progress to Done in Architecture Review Apr 10, 2025

thenguyenyf mentioned this pull request Apr 25, 2025

drivers: udc: renesas: enable high-speed USB support #89064

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb: device_next: Add Kconfig to set maximum speed #76255

usb: device_next: Add Kconfig to set maximum speed #76255

tmon-nordic commented Jul 24, 2024 •

edited

Loading

henrikbrixandersen Jul 24, 2024

tmon-nordic Jul 24, 2024

henrikbrixandersen Jul 24, 2024

jfischer-no Jul 24, 2024

tmon-nordic Jul 24, 2024

tmon-nordic Jul 24, 2024

jfischer-no Jul 24, 2024

tmon-nordic Jul 24, 2024

tmon-nordic Jul 25, 2024

bjarki-andreasen left a comment

tmon-nordic commented Aug 1, 2024

tmon-nordic commented Aug 28, 2024

tmon-nordic commented Mar 5, 2025

henrikbrixandersen commented Mar 5, 2025

carlescufi commented Mar 11, 2025 •

edited

Loading

jfischer-no commented Mar 11, 2025

henrikbrixandersen commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

cfriedt commented Mar 11, 2025

decsny commented Mar 11, 2025

decsny commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

tmon-nordic commented Mar 11, 2025 •

edited

Loading

decsny commented Mar 11, 2025 •

edited

Loading

tmon-nordic commented Mar 12, 2025 •

edited

Loading

tmon-nordic commented Apr 7, 2025

usb: device_next: Add Kconfig to set maximum speed #76255

usb: device_next: Add Kconfig to set maximum speed #76255

Conversation

tmon-nordic commented Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarki-andreasen left a comment

Choose a reason for hiding this comment

tmon-nordic commented Aug 1, 2024

tmon-nordic commented Aug 28, 2024

tmon-nordic commented Mar 5, 2025

henrikbrixandersen commented Mar 5, 2025

carlescufi commented Mar 11, 2025 • edited Loading

jfischer-no commented Mar 11, 2025

henrikbrixandersen commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

cfriedt commented Mar 11, 2025

decsny commented Mar 11, 2025

decsny commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

jfischer-no commented Mar 11, 2025

tmon-nordic commented Mar 11, 2025 • edited Loading

decsny commented Mar 11, 2025 • edited Loading

tmon-nordic commented Mar 12, 2025 • edited Loading

tmon-nordic commented Apr 7, 2025

tmon-nordic commented Jul 24, 2024 •

edited

Loading

carlescufi commented Mar 11, 2025 •

edited

Loading

tmon-nordic commented Mar 11, 2025 •

edited

Loading

decsny commented Mar 11, 2025 •

edited

Loading

tmon-nordic commented Mar 12, 2025 •

edited

Loading