drivers/periph_usbdev: fix set device address #19471

gschorcht · 2023-04-15T10:41:17Z

Contribution description

This PR allows to define when the device address is set on receipt of a SETUP with SET ADDRESS Request. It fixes the problem with enumeration of the Synopsys DWC2 USB OTG Core due to the wrong time of setting the device address.

Especially, it fixes the problem that the enumeration fails completely for the stm32f723e-disco board with CDC ECM if CDC ACM is not used and the additional reset cycles during the enumeration for a couple of platforms such as ESP32-S2 and ESP32-S3.

Background

The address in the USB device can be set either directly after the SETUP stage on receipt of the SET ADDRESS Request or after the associated STATUS stage. When the USB device address has to be set depends on the hardware implementation.
Solution

To control the time of setting the device address, a new define USBDEV_SET_ADDR_AFTER_STATUS is introduced.
If USBDEV_SET_ADDR_AFTER_STATUS has the value 1 (default), the address is set in the USB device after the STATUS stage. Since this is the default, existing periph_usbdev drivers shouldn't be affected. Overwriting USBDEV_SET_ADDR_AFTER_STATUS with 0 in periph_cpu.h or in driver header file let the address set directly after the SETUP stage.

Testing procedure

Use tests/usbus_cdc_ecm:

For stm32f723e-disco the enumeration doesn't work at all without this PR and works reliable with this PR.

USEMODULE='periph_usbdev_hs_utmi' BOARD=stm32f723e-disco make -C tests/usbus_cdc_ecm flash

For any ESP32-S2 or ESP32-S3 board, the enumeration requires an addition reset cycle in every third or fourth enumeration without this PR and doesn't require any reset cycle with this PR.

BOARD=esp32s2-devkit make -C tests/usbus_cdc_ecm flash

Other platforms should still work with this PR, for example ATSAM platform:

BOARD=arduino-mkr1000 make -C tests/usbus_cdc_ecm flash

Issues/PRs references

riot-ci · 2023-04-15T10:53:02Z

Murdock results

✔️ PASSED

88cabba drivers/usbdev_synopsys_dwc2: set USBDEV_SET_ADDR_AFTER_STATUS

Success	Failures	Total	Runtime
6882	0	6882	10m:05s

Artifacts

Documentation preview

The address in the USB device can be set either directly after the SETUP stage on receipt of the `SET ADDRESS Request` or after the associated status stage. When the USB device address has to be set depends on the hardware. If `USBDEV_SET_ADDR_AFTER_STATUS` has the value 1 (default), the address is only set in the USB device after the status stage. Overwrite it with 0 in `periph_cpu.h` to set the address already directly after the SETUP stage. sys/usbus: use USBDEV_SET_ADDR_AFTER_STATUS Use symbol `USBDEV_SET_ADDR_AFTER_STATUS` to determine whether the device address has to be set directly after SETUP stage or after the associated STATUS stage. drivers/usbdev_synopsys_dwc2: set USBDEV_SET_ADDR_AFTER_STATUS DWC2 core requires that the device address has to be set directly after SETUP stage and not after the associated STATUS stage.

bergzand · 2023-04-15T17:50:23Z

sys/usb/usbus/usbus_control.c

@@ -241,6 +241,10 @@ static int _recv_dev_setup(usbus_t *usbus, usb_setup_t *pkt)
            case USB_SETUP_REQ_SET_ADDRESS:
                DEBUG("usbus_control: Setting address\n");
                usbus->addr = (uint8_t)pkt->value;
+                if (!IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {


Suggested change

if (!IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {

if (USBDEV_SET_ADDR_AFTER_STATUS) {

Is sufficient if USBDEV_SET_ADDR_AFTER_STATUS is always set.

bergzand · 2023-04-15T17:50:36Z

sys/usb/usbus/usbus_control.c

@@ -401,8 +405,10 @@ static int _handle_tr_complete(usbus_t *usbus,
        case USBUS_CONTROL_REQUEST_STATE_INACK:
            if (ep->dir == USB_EP_DIR_IN) {
                if (usbus->addr && usbus->state == USBUS_STATE_RESET) {
-                    usbdev_set(usbus->dev, USBOPT_ADDRESS, &usbus->addr,
-                               sizeof(usbus->addr));
+                    if (IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {


Suggested change

if (IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {

if (USBDEV_SET_ADDR_AFTER_STATUS) {

Same as above

bergzand · 2023-04-15T17:50:41Z

drivers/include/periph/usbdev.h

+ * in `periph_cpu.h` to set the address already directly after the SETUP stage.
+ */
+#ifndef USBDEV_SET_ADDR_AFTER_STATUS
+#define USBDEV_SET_ADDR_AFTER_STATUS    1


I think you want to include periph_conf.h in this file to avoid conflicts when periph_conf.h overrides this setting, but is included after this file.

Since it is a property of the MCU and not of a board, it would be defined in periph_cpu.h if it is defined. The file periph_cpu.h is already included. So I wondered if I should call the symbol USBDEV_CPU_SET_ADDR_AFTER_STATUS instead.

Setting it in periph_cpu.h sounds perfect. I don't have a strong opinion on the name, but using the USBDEV_CPU_ prefix sounds nice as that aligns it with the USBDEV_CPU_DMA_* macros.

usbdev_synopsys_dwc2.h (that overrides this) is as far as I can see not included in the stm32 periph_cpu.h, so depending on the other includes in the file, it either does or does not override this define for that peripheral.

usbdev_synopsys_dwc2.h (that overrides this) is as far as I can see not included in the stm32 periph_cpu.h

By intention because of USB cores and the different drivers used. It's included instead in boards/common/stm32/cfg_usb_otg_*.h. usbdev_fs requires to set the device address after STATUS stage.

The question is how we can ensure that the definition in usbdev_synopsys_dwc2.h always comes before the default defintion in periph/usbdev.h

I changed the name to USBDEV_CPU_SET_ADDR_AFTER_STATUS.

The question is how we can ensure that the definition in usbdev_synopsys_dwc2.h always comes before the default defintion in periph/usbdev.h

That's why I was suggesting to include periph_conf.h in periph/usbdev.h. periph_conf.h must include the usbdev_synopsys_dwc2.h if it configures one, pretty much guaranteeing the correct order of includes required here. Other periph headers (periph/spi.h and periph/i2c.h) files also include periph_conf.h, so I don't see much reason why not to include it also here.

bergzand · 2023-04-16T15:09:43Z

Please squash!

The address in the USB device can be set either directly after the SETUP stage on receipt of the `SET ADDRESS Request` or after the associated status stage. When the USB device address has to be set depends on the hardware. If `USBDEV_SET_ADDR_AFTER_STATUS` has the value 1 (default), the address is only set in the USB device after the status stage. Overwrite it with 0 in `periph_cpu.h` to set the address already directly after the SETUP stage.

Use symbol `USBDEV_SET_ADDR_AFTER_STATUS` to determine whether the device address has to be set directly after SETUP stage or after the associated STATUS stage.

DWC2 core requires that the device address has to be set directly after SETUP stage and not after the associated STATUS stage.

bergzand

Ack!

bors merge

bors · 2023-04-16T17:36:25Z

Build succeeded:

gschorcht · 2023-04-16T20:57:09Z

Thanks

gschorcht requested review from bergzand, dylad, aabadie and MrKevinWeiss as code owners April 15, 2023 10:41

github-actions bot added Area: drivers Area: Device drivers Area: sys Area: System Area: USB Area: Universal Serial Bus labels Apr 15, 2023

gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 15, 2023

gschorcht added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 15, 2023

gschorcht force-pushed the drivers/periph_usbdev_fix_set_addr branch from 35f6fe1 to 0635f27 Compare April 15, 2023 14:37

riot-ci mentioned this pull request Apr 15, 2023

drivers/usbdev_synopsys_dwc2: add GD32V support #19389

Merged

bergzand requested changes Apr 15, 2023

View reviewed changes

gschorcht added 3 commits April 16, 2023 17:14

sys/usbus: use USBDEV_SET_ADDR_AFTER_STATUS

e46fa4a

Use symbol `USBDEV_SET_ADDR_AFTER_STATUS` to determine whether the device address has to be set directly after SETUP stage or after the associated STATUS stage.

drivers/usbdev_synopsys_dwc2: set USBDEV_SET_ADDR_AFTER_STATUS

88cabba

DWC2 core requires that the device address has to be set directly after SETUP stage and not after the associated STATUS stage.

gschorcht force-pushed the drivers/periph_usbdev_fix_set_addr branch from c323a13 to 88cabba Compare April 16, 2023 15:14

bergzand approved these changes Apr 16, 2023

View reviewed changes

bors bot merged commit 1961274 into RIOT-OS:master Apr 16, 2023

gschorcht deleted the drivers/periph_usbdev_fix_set_addr branch April 26, 2023 09:40

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

drivers/periph_usbdev: fix set device address #19471

drivers/periph_usbdev: fix set device address #19471

Uh oh!

gschorcht commented Apr 15, 2023

Uh oh!

riot-ci commented Apr 15, 2023 •

edited

Loading

Uh oh!

bergzand Apr 15, 2023

Uh oh!

bergzand Apr 15, 2023

Uh oh!

bergzand Apr 15, 2023

Uh oh!

gschorcht Apr 16, 2023

Uh oh!

bergzand Apr 16, 2023

Uh oh!

gschorcht Apr 16, 2023

Uh oh!

gschorcht Apr 16, 2023

Uh oh!

bergzand Apr 16, 2023

Uh oh!

bergzand commented Apr 16, 2023

Uh oh!

bergzand left a comment

Uh oh!

bors bot commented Apr 16, 2023

Uh oh!

gschorcht commented Apr 16, 2023

Uh oh!

Uh oh!

	if (!IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {
	if (USBDEV_SET_ADDR_AFTER_STATUS) {

	if (IS_ACTIVE(USBDEV_SET_ADDR_AFTER_STATUS)) {
	if (USBDEV_SET_ADDR_AFTER_STATUS) {

drivers/periph_usbdev: fix set device address #19471

drivers/periph_usbdev: fix set device address #19471

Uh oh!

Conversation

gschorcht commented Apr 15, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

riot-ci commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

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!

bergzand commented Apr 16, 2023

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Apr 16, 2023

Uh oh!

gschorcht commented Apr 16, 2023

Uh oh!

Uh oh!

riot-ci commented Apr 15, 2023 •

edited

Loading