Skip to content
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

feature(dcd_dwc2) : Added cache synchronization on esp32p4 while DMA is used #37

Closed
wants to merge 9 commits into from

Conversation

roma-jam
Copy link
Collaborator

@roma-jam roma-jam commented Nov 11, 2024

Requirements

  1. To enable Buffer DMA mode, esp-tinyusb changes should be done (define CFG_TUD_DWC2_DMA and implement CFG_TUSB_MEM_SECTION, refer feature(esp_tinyusb): Added mode configuration for dcd_dwc2 layer esp-usb#88)
  2. Support Buffer DMA mode for ESP32S2/S3 (no sync required, so this could be achieved with step 1.) and ESP32P4 (sync required, changes in this PR is crucial for Buffer DMA on P4) chips.

Measurements

Measurements were made on tusb_msc example (read-write operations benchmark, SDMMC target).

ESP32S3

MSC Buffer size (bytes) Slave Mode (R/W) Buffer DMA (R/W)
512 463,5 kB/s / 161,1 kB/s 512,6 kB/s / 171,2 kB/s
4096 733,5 kB/s / 452,9 kB/s 878,8 kB/s / 469,9 kB/s
8192 N/A / N/A N/A / N/A

ESP32P4

MSC Buffer size (bytes) Slave Mode (R/W) Buffer DMA (R/W)
512 909,6 kB/s / 217,8 kB/s 942,8 kB/s / 222,6 kB/s
4096 932,7 kB/s / 225,2 kB/s 3,8 MB/s / 1,1 MB/s
8192* 934,4 KB/s / 229,4 KB/s 5,3 MB/s / 1,8 MB/s

*Benchmark results provided

image

image

Limitations

Not working:

  • tusb_ncm (S2/S3, TinyUSB stack overflow during recv_transfer_datagram_to_glue_logic())

Working examples:

  • tusb_composite_msc_serial
  • tusb_console
  • tusb_hid
  • tusb_midi
  • tusb_msc
  • tusb_serial_device (verified tx with len = [1..512])

Breaking change

No breaking changes

Checklist

  • Pull Request name has appropriate format (for example: "fix(dcd_dwc2): Resolved address selection when several phy are present")
  • Optional: README.md updated
  • CI passing

Related issues

No related issues

@roma-jam roma-jam self-assigned this Nov 11, 2024
@roma-jam roma-jam changed the base branch from master to release/v0.17 November 11, 2024 13:46
@roma-jam roma-jam force-pushed the feature/esp32p4_dma_cache_sync branch from 8b93c5c to f4ffca9 Compare November 14, 2024 11:39
@roma-jam roma-jam changed the title feature(dcd_dwc2) : Added DMA cache syncronization on esp32p4 feature(dcd_dwc2) : Added DMA cache synchronization on esp32p4 Nov 14, 2024
@roma-jam roma-jam force-pushed the feature/esp32p4_dma_cache_sync branch 2 times, most recently from a9acc7f to 1f85a0e Compare November 18, 2024 20:14
@roma-jam roma-jam force-pushed the feature/esp32p4_dma_cache_sync branch from 1f85a0e to 3fb9c86 Compare November 19, 2024 11:25
@roma-jam roma-jam marked this pull request as ready for review November 19, 2024 11:56
@roma-jam
Copy link
Collaborator Author

@tore-espressif @peter-marcisovsky
This is a (preliminary version with using cache sync) DMA support for P4.
There are couple of changes, regarding the MSC and CDC class drivers, so let me know if it is better to move them out to another PR.

DMA requires the specific configuration by esp-tinyusb component, so the link to the changes: espressif/esp-usb#88

Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Just a few work-in-progress comments/questions

@@ -848,6 +883,11 @@ static void handle_epout_dma(uint8_t rhport, uint8_t epnum, dwc2_doepint_t doepi

if (doepint_bm.setup_phase_done) {
dma_setup_prepare(rhport);
// CACHE HINT
// When cache is enabled, _setup_packet must have cache line size alignment
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the terminology used in this PR.

What does it mean 'cache is enabled'? We sure don't want to enable/disable the cache for all RAM access system wide...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It more like about the way, how we want to handle the memory. while DMA is used.
I thought, that there will be an option to choose between:

  • Use cache and synchronize it explicitly in dcd_dwc2
  • Bypass cache and use direct-access addresses

src/portable/synopsys/dwc2/dwc2_esp32.h Outdated Show resolved Hide resolved
src/portable/synopsys/dwc2/dwc2_esp32.h Outdated Show resolved Hide resolved
src/portable/synopsys/dwc2/dwc2_esp32.h Outdated Show resolved Hide resolved
src/portable/synopsys/dwc2/dwc2_esp32.h Outdated Show resolved Hide resolved
src/portable/synopsys/dwc2/dwc2_esp32.h Outdated Show resolved Hide resolved
@roma-jam roma-jam marked this pull request as draft November 19, 2024 22:24
@roma-jam roma-jam changed the title feature(dcd_dwc2) : Added DMA cache synchronization on esp32p4 feature(dcd_dwc2) : Added cache synchronization on esp32p4 while DMA is used Nov 20, 2024
@roma-jam roma-jam force-pushed the feature/esp32p4_dma_cache_sync branch from 3fb9c86 to d073592 Compare November 20, 2024 11:52
@roma-jam roma-jam marked this pull request as ready for review November 20, 2024 12:19
@roma-jam
Copy link
Collaborator Author

This PR is not relevant anymore, as the cache support were implemented in upstream based on the partial commit from here (hathach#2877)
The device classes modifications for buffer alignment and memory sections is in the upstream PR: hathach#2884

So far, temporary changes in class device (cdc and mscd) are not significant, as they will be covered by the merging the PR above.

@roma-jam roma-jam closed this Nov 22, 2024
@peter-marcisovsky
Copy link
Collaborator

@roma-jam sorry for the late review, I see, it closed now..

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.

3 participants