espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.79k stars 7.31k forks source link

Incorrect register field definitions in soc/usb_reg.h (IDFGH-11591) #12710

Open simpkins opened 11 months ago

simpkins commented 11 months ago

Answers checklist.

General issue report

It looks like a few of the endpoint register field definitions in components/soc/esp32s3/include/soc/usb_reg.h are likely incorrect:

USB_D_XFERSIZEn

USB_D_XFERSIZE1, USB_D_XFERSIZE2, USB_D_XFERSIZE3, USB_D_XFERSIZE4, USB_D_XFERSIZE5, and USB_D_XFERSIZE6 are all defined as 0x7F, but it looks like these values should probably be 0x7FFFF. The associated USB_D_XFERSIZE1_V definitions appear to be the same. The value 0x7F appears to be correct for endpoint 0, but it seems like this value should be larger for other endpoints. I suspect the value from endpoint 0 was incorrectly copied to the other endpoints.

The ESP32-S3 Technical Reference Manual unfortunately does not provide documentation for these registers. However, the tinyusb code provided for the ESP32-S3 is explicitly using a value of 0x7FFFF instead of using these definitions from soc/usb_reg.h for this value. It seems like the ESP32 USB core is based on a Synopsys core, and other vendors using synposys cores document these values as 0x7F for endpoint 0 but 0x7FFFF for other endpoints.

The USB_XFERSIZEn definitions (without a _D_ in the name) appear to have the same issue.

USB_D_MPSx

This value similarly seems incorrect for endpoints 1 through 6, and the values for endpoint 0 appear to have been incorrectly copied to other endpoints. USB_D_MPS0 is correctly defined as 3, but for the other endpoints it looks like this value should be 0x7FF.

The comments describing the values for this field also appear incorrect: this comment describes the values for endpoint 0, but for all other endpoints this field should be set to the literal maximum packet size value.

Again, the tinyusb code for the ESP32 appears to be doing the right thing here, and simply ignoring the incorrect values for from soc/usb_reg.h

Note that the USB_MPSx definitions do appear to be correct: 3 for endpoint 0 and 0x7FF for other endpoints.

USB_D_PKTCNTx

As above, this is defined as 3 for all endpoints, and I suspect that this value is correct for endpoint 0 but should be larger for other endpoints. Other vendors using synopsys USB cores document this value as 3 for endpoint 0, but 0x3FF for other endpoints.

I suspect the same issue exists for the USB_PKTCNTx values.

tore-espressif commented 11 months ago

Hello @simpkins , yes, you're right. This header file is automatically generated to describe all USB-OTG registers. As you noted, we don't use this definitions in our TinyUSB DCD layer. We will fix these inaccuracies, when we implement new (DMA based) DCD layer