Skip to content

host/dwc2: fix bitfields access width #3070

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.

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

Merged
merged 7 commits into from
Apr 15, 2025
Merged

Conversation

HiFiPhile
Copy link
Collaborator

@HiFiPhile HiFiPhile commented Apr 7, 2025

Describe the PR
Fix gcc-riscv issue about volatile bitfields are incorrectly accessed using an access width not appropriate to the type of its container.

Tested on espressif_s3_devkitm with device_info and cdc_msc_freertos examples.

For the host I've tested an usb key and an FTDI usb-uart.

Device 1: ID 0403:6001 SN FT1ZK9MQ
Device Descriptor:
  bLength             18
  bDescriptorType     1
  bcdUSB              0200
  bDeviceClass        0
  bDeviceSubClass     0
  bDeviceProtocol     0
  bMaxPacketSize0     8
  idVendor            0x0403
  idProduct           0x6001
  bcdDevice           0600
  iManufacturer       1     FTDI
  iProduct            2     TTL232R-3V3
  iSerialNumber       3     FT1ZK9MQ
  bNumConfigurations  1

Device 2: ID 0011:7788 SN C4D197AC
Device Descriptor:
  bLength             18
  bDescriptorType     1
  bcdUSB              0200
  bDeviceClass        0
  bDeviceSubClass     0
  bDeviceProtocol     0
  bMaxPacketSize0     64
  idVendor            0x0011
  idProduct           0x7788
  bcdDevice           0104
  iManufacturer       1     Generic
  iProduct            2     Mass Storage
  iSerialNumber       3     C4D197AC
  bNumConfigurations  1

Disconnection detection needs #2960 to work otherwise device index will go up 1,2,3,4...

Fix #3047

@HiFiPhile HiFiPhile changed the title Bitfield host/dwc2: fix bitfields access width Apr 7, 2025
@hathach
Copy link
Owner

hathach commented Apr 8, 2025

thank you, I am thinking, maybe we just remove the struct/bitfield in the register typedef i.e leaving only uint32_t. bitfield struct can be use with local variale only. Will try that when I got sometime,

@HiFiPhile
Copy link
Collaborator Author

I am thinking, maybe we just remove the struct/bitfield in the register typedef i.e leaving only uint32_t. bitfield struct can be use with local variable only

Yes we can do let this. Adding a macro makes the thing complicate and losing the interest of bitfields.

@hathach
Copy link
Owner

hathach commented Apr 8, 2025

I am thinking, maybe we just remove the struct/bitfield in the register typedef i.e leaving only uint32_t. bitfield struct can be use with local variable only

Yes we can do let this. Adding a macro makes the thing complicate and losing the interest of bitfields.

sure, I will do this later if you are busy. I need to fix an racing issue with pio-usb when overclocking rp2350 to 264mhz first :)

Signed-off-by: HiFiPhile <[email protected]>
Signed-off-by: HiFiPhile <[email protected]>
Signed-off-by: HiFiPhile <[email protected]>
@HiFiPhile
Copy link
Collaborator Author

I've updated the refactor.

Signed-off-by: HiFiPhile <[email protected]>
Signed-off-by: HiFiPhile <[email protected]>
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

perfect, thank you. I couldn't done it any better. I change dwc2_ep/channel_count() to inline function to avoid forcing gnu standard. The rest is mostly clean up

@hathach hathach merged commit bfe0817 into hathach:master Apr 15, 2025
129 of 130 checks passed
@HiFiPhile HiFiPhile deleted the bitfield branch April 22, 2025 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

freertos reset with ESP_RST_INT_WDT on esp32-s3 when using option CFG_TUH_DWC2_SLAVE_ENABLE
2 participants