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

feat(Examples,PeriphDrivers): Add SPIMSS_DMA example and SPIMSS Master Trans DMA APIs #840

Conversation

kilicomercan
Copy link
Contributor

@kilicomercan kilicomercan commented Dec 19, 2023

Description

There is no transaction API over DMA for SPIMSS driver in MSDK. In addition, we don't have example application which shows how to make SPIMSS master transaction by using DMA.

Implemented Example:
SPIMSS_DMA example code is added for MAX32660 EVSYS

Implemented API Features:
SPIMSS DMA "Auto handlers". Auto handling is disabled by default.
APIs for using application defined DMA handlers and channels.

New API for SPIMSS Master Dma Transaction:
int MXC_SPIMSS_MasterTransDMA(mxc_spimss_regs_t *spi, mxc_spimss_req_t *req);

New APIs for Assigning DMA Channels:
int MXC_SPIMSS_SetTXDMAChannel(mxc_spimss_regs_t *spi, unsigned int channel);
int MXC_SPIMSS_SetRXDMAChannel(mxc_spimss_regs_t *spi, unsigned int channel);
int MXC_SPIMSS_GetTXDMAChannel(mxc_spimss_regs_t *spi);
int MXC_SPIMSS_GetRXDMAChannel(mxc_spimss_regs_t *spi);

@github-actions github-actions bot added the MAX32660 Related to the MAX32660 (ME11) label Dec 19, 2023
@kilicomercan kilicomercan marked this pull request as draft December 19, 2023 17:21
@kilicomercan kilicomercan marked this pull request as ready for review December 19, 2023 17:24
@kilicomercan kilicomercan reopened this Dec 19, 2023
@kilicomercan kilicomercan reopened this Dec 20, 2023
@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch 4 times, most recently from 37b7f58 to d692efd Compare December 20, 2023 10:09
Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @kilicomercan

Consider the case where DMA channel 0 and 1 are already in use. In this case, the transaction function will acquire channel 2/3, and the ISRs and handlers will never be triggered (even though you've released the channel in the example code, we can't expect application code to also do this). Acquiring the DMA channel inside the transaction function is convenient, but it becomes problematic. We can't guarantee that it will acquire a specific channel, so we can't always assign the DMA_0_-specific handler. We also cannot easily check which channels will be used from "outside" the function in order to assign the correct ISRs, because the transaction acquires them on the fly.

I solved this problem for the UART module by implementing an "auto handlers" feature, as well as an API for users who would like to manually assign and manage their DMA channels.

I think a similar approach would solve this problem here as well. Take a look at #763 for more details

Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch from d692efd to 2f9024b Compare December 27, 2023 16:04
@github-actions github-actions bot added the MAX32650 Related to the MAX32650 (ME10) label Dec 27, 2023
@kilicomercan kilicomercan changed the title feat(Examples,PeriphDrivers): Add SPIMSS_DMA example and MXC_SPIMSS_MasterTransDMA API feat (Examples,PeriphDrivers): Add SPIMSS_DMA example and SPIMSS Master Trans DMA APIs Dec 27, 2023
@kilicomercan kilicomercan changed the title feat (Examples,PeriphDrivers): Add SPIMSS_DMA example and SPIMSS Master Trans DMA APIs feat(Examples,PeriphDrivers): Add SPIMSS_DMA example and SPIMSS Master Trans DMA APIs Dec 27, 2023
@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch 5 times, most recently from 7e7e878 to 022d621 Compare December 27, 2023 17:09
Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Thanks @kilicomercan, the new APIs for the DMA handlers look good.

I have some more comments below. They should be easy to fix, but there is one more detailed question related to the 16-bit data alignment.

Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch from aa2083c to 8ae3990 Compare January 11, 2024 15:17
Copy link
Contributor

@sihyung-maxim sihyung-maxim left a comment

Choose a reason for hiding this comment

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

Thanks for your changes! I like this specific approach of having the option to set up DMA beforehand instead of within the transaction function itself.

Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Examples/MAX32660/SPIMSS_DMA/main.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Libraries/PeriphDrivers/Source/SPIMSS/spimss_reva.c Outdated Show resolved Hide resolved
Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Looks good @kilicomercan, thanks. Sihyung has some review notes as well

@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch 3 times, most recently from 9d40580 to e90e6e6 Compare January 17, 2024 10:42
@kilicomercan
Copy link
Contributor Author

Hi @Jake-Carter and @sihyung-maxim ,
Thank you for your comments. I pushed the latest changes to PR. Could you please review it again?

@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch from e90e6e6 to 02c081c Compare January 24, 2024 07:13
@kilicomercan kilicomercan force-pushed the example/max32660_spimss_master_dma branch from 2a9a235 to b2f9044 Compare January 25, 2024 05:54
Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Nice work on a big driver addition!

@Jake-Carter Jake-Carter merged commit 91782bc into analogdevicesinc:main Jan 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAX32650 Related to the MAX32650 (ME10) MAX32660 Related to the MAX32660 (ME11)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants