Skip to content

mspm0: add dma driver #4338

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

mspm0: add dma driver #4338

wants to merge 1 commit into from

Conversation

i509VCB
Copy link
Member

@i509VCB i509VCB commented Jun 24, 2025

Add a DMA driver for mspm0. So far only implement features available on all (basic) channels. copying slices is also not implemented yet nor anything related to source/destination stride.

A few things pending before this is ready to review

  • merge dma: Apply MakeFieldArray to interrupt mspm0-rs/mspm0-data#6
  • add tests for u16, u32 and u64 transfers
  • Figure out why TransferMode::Single with dst.len() > 2 performs 2 transfers instead of 1 (as expected in datasheet) (likely hardware bug)
  • Test multiple triggers (resume())
  • Remove the settings.json changes (after I am done)

@optlink
Copy link
Contributor

optlink commented Jun 27, 2025

I do have some experience with this DMA controller in a real application and I think your issue might be the software trigger. With an external trigger from the ADCs the single transfer mode works as expected. I have not verified this myself but I suspect that the software trigger just holds the trigger active and makes the transfer mode redundant given that I see no actual way to control the software trigger.

@i509VCB
Copy link
Member Author

i509VCB commented Jun 27, 2025

Yeah it does feel like software triggering the dma doesn't behave right. If I set the destination to 4 bytes in single transfer mode 2 bytes are written as well. That doesn't match the idea of the software trigger letting the DMA run to completion.

This feels like a hardware bug or the datasheet is too ambigious to answer?

@optlink
Copy link
Contributor

optlink commented Jun 28, 2025

Hmm that is strange. Based on the software trigger behavior of the ADC I expected it to be similar. It does seem like a pathological case though. When would you use such a configuration in a real application? Perhaps it’s considered undefined behavior and just not documented very well like most of this series.

@i509VCB
Copy link
Member Author

i509VCB commented Jun 28, 2025

When would you use such a configuration in a real application?

Probably not practical to do single software triggered single transfers for an array of reads (basically using the CPU for memcpy with extra steps). Exhaustively testing the driver options is why that test case came to exist.

@optlink
Copy link
Contributor

optlink commented Jun 28, 2025

Fair enough and that’s what I guessed was happening. The rest of the driver matches what I know of this module so I think it’s sound except for this one test case.

@i509VCB i509VCB force-pushed the dma branch 3 times, most recently from 66f19f4 to 1b03d0f Compare June 28, 2025 05:20
@i509VCB i509VCB marked this pull request as ready for review June 28, 2025 05:25
Copy link

@Iooon Iooon left a comment

Choose a reason for hiding this comment

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

More questions than change requests.
Very nice to see dma advancing and also to have the bind_interrupt macro :)

}
}

// TODO: u128 (LONGLONG) support is available but not on every part number (do any currently released
Copy link

Choose a reason for hiding this comment

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

Doesn't it say in the G3507 reference that it supports u128? Maybe I am wrong
Screenshot_20250630_221150_Firefox.jpg

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay so there is a target. I probably need to find some metadata info so that I can feature gate u128 transfers to parts that support it.

let events = pac::DMA.int_event(0);
let mis = events.mis().read();

// TODO: Handle DATAERR and ADDRERR? However we do not know which channel causes an error.
Copy link

Choose a reason for hiding this comment

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

Am I correct that we need to handle this on the channel level, because of the way the chip raises the error?
So each channel would need to check the IIDX[j].STAT register individually?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the array access for int_event is selecting the "CPU event" interrupts. Index 1 for int_event is the GEN_EVENT group.

Looking at the STAT values (and it matches with INT):
image

We see that the DMA address and data errors do not specify what channel caused the error. We can be told of course when a channel finishes, but if an error happens we don't know which channel failed.

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.

3 participants