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

Rework DMA descriptors #1054

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Rework DMA descriptors #1054

merged 1 commit into from
Jan 30, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Dec 29, 2023

The goal of this PR is to better model DMA descriptors by using a 3-word struct and aligning documentation. As a bonus, we can remove a few runtime checks.

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@bugadani bugadani force-pushed the desc branch 5 times, most recently from d3e117f to 165820f Compare December 29, 2023 21:28
@sourcebox
Copy link

Descriptors should be sized as `(CHUNK_SIZE + 4091) / 4092

I think "should be sized as CHUNK_SIZE / 4096 + 1" is a more clear formula.

Btw. the values 4091 and 4092 are probably wrong anyway because 2^12 = 4096.

@bugadani
Copy link
Contributor Author

bugadani commented Dec 29, 2023

I think "should be sized as CHUNK_SIZE / 4096 + 1" is a more clear formula.

I know the difference is only occasional and only 12 bytes, but why waste it? If I only ever need to transfer a single chunk, I don't want 2 descriptors. This means the macro will stay most likely as it is currently, which in turn would mean the documentation would be incorrect (again).

wrong

I didn't invent it, I don't feel confident to change it. I don't know what needs and what doesn't need 4 byte alignment (something requiring word alignment means we can't use 4095 byte chunks), but if you feel compelled to figure it out, change it and test it, feel free to open a PR.

@sourcebox
Copy link

Section 3.4.8 of the TRM: "That is to say, GDMA can read data of specified length (1 ~ 4095 bytes) from any
start addresses in the accessible address range, or write received data of the specified length (1 ~ 4095 bytes) to
any contiguous addresses in the accessible address range."

So where do these 4091/4092 numbers come from?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 2, 2024

I don't remember where these number are from but there are things like these:

https://github.com/espressif/esp-idf/blob/5d8fb281e7b4ae3f3827c91aa6dc7ef969580b5a/components/driver/deprecated/i2s_legacy.c#L498

https://github.com/espressif/esp-idf/blob/5d8fb281e7b4ae3f3827c91aa6dc7ef969580b5a/components/driver/deprecated/i2s_legacy.c#L82

Anyways it shouldn't get changed in a walk-by - if it turns out 4095/4096 is fine it should be another PR

@sourcebox
Copy link

Interestingly, the numbers only appear in the doc comments. There's nothing in the code that checks against them.

@bugadani
Copy link
Contributor Author

bugadani commented Jan 2, 2024

Just out of curiousity, what do you expect to happen, when the chunk size is 4095 bytes, but a peripheral supports only 32bit data?

@sourcebox
Copy link

Just out of curiousity, what do you expect to happen, when the chunk size is 4095 bytes, but a peripheral supports only 32bit data?

Good question. Seems to be similar to any case where chunk size and peripheral data size are not aligned, e.g. on a 3 byte transfer with 32-bit peripheral size. How does the hardware behave in such a case? I would guess, transfer would stall and wait for the missing byte. But that is something that should be prevented, so there has to be some modulo calculation depending on the data width.

@bugadani

This comment was marked as outdated.

@bugadani bugadani marked this pull request as ready for review January 29, 2024 20:33
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM!

@bjoernQ bjoernQ added this pull request to the merge queue Jan 30, 2024
Merged via the queue into esp-rs:main with commit deb9f21 Jan 30, 2024
17 checks passed
@bugadani bugadani deleted the desc branch January 30, 2024 08:05
playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants