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.

Sign up for GitHub

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

Jump to bottom

cpu/samd5x: add SD Host Controller implementation #17863

Merged
merged 2 commits into from
May 31, 2022
Merged

cpu/samd5x: add SD Host Controller implementation #17863

merged 2 commits into from
May 31, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Mar 25, 2022 •
edited
Loading

Contribution description

This implements support for the SD/MMC host controller found on SAM D5x/E5x family chips.
It is based on the code from initmaker (and the accompanying blog post) with interrupt support instead of busy-waiting.

Testing procedure

Insert an SD card into the SD card slot of the same54-xpro

tests/mtd_raw should work:

2022-03-25 11:57:43,168 # main(): This is RIOT! (Version: 2022.04-devel-912-g8732f-sam0_sdhc)
2022-03-25 11:57:43,169 # Manual MTD test
2022-03-25 11:57:43,173 # init MTD_0… OK (32 MiB)
2022-03-25 11:57:43,176 # init MTD_1… OK (256 byte)
2022-03-25 11:57:43,210 # init MTD_2… OK (7580 MiB)
> test 2
2022-03-25 11:57:44,557 # test 2
2022-03-25 11:57:44,558 # [START]
2022-03-25 11:57:44,588 # [SUCCESS]

We have an internal benchmark tool that generates data records to and stores them on the SD card, there we see improvements compared to sdcard_spi:

sdcard_spi

2022-03-25 17:30:01,204 # bench 1000
2022-03-25 17:30:06,203 # write:   4991646us  ---  4991.646us per call  ---        200 calls per sec

sam0_sdhc

2022-03-25 12:42:14,891 # bench 1000
2022-03-25 12:42:17,130 # write:   2230878us  ---  2230.878us per call  ---        448 calls per sec

Issues/PRs references

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Mar 26, 2022
dylad
dylad reviewed Mar 26, 2022
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Awesome PR ! That's something I had on my todolist for ages...
I just gave it a quick look. I'll look more in details later.
tests/mtd_raw works as expected.
I hope I can test it alongside with USB Mass storage at some point :)

cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Show resolved Hide resolved
@github-actions github-actions bot added the Area: sys Area: System label Mar 29, 2022
dylad
dylad reviewed Mar 31, 2022
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

next round of review.

cpu/sam0_common/include/sdhc.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/sdhc.h Show resolved Hide resolved
cpu/sam0_common/include/sdhc.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/sdhc.h Outdated Show resolved Hide resolved
cpu/sam0_common/include/sdhc.h Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/mtd_sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/mtd_sdhc.c Outdated Show resolved Hide resolved
@benpicco benpicco mentioned this pull request Apr 11, 2022
Merged
@github-actions github-actions bot removed Area: drivers Area: Device drivers Area: tests Area: tests and testing framework labels Apr 12, 2022
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Apr 12, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 29, 2022
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels Apr 29, 2022
@benpicco benpicco requested a review from maribu May 24, 2022 08:07
maribu
maribu reviewed May 24, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Code looks good. Some comments inline. You may squash directly in the changes.

cpu/sam0_common/sam0_sdhc/sdhc.c
Comment on lines +118 to +123
if (IS_USED(MODULE_ZTIMER_USEC)) {
ztimer_sleep(ZTIMER_USEC, us);
} else if (IS_USED(MODULE_ZTIMER_MSEC)) {
ztimer_sleep(ZTIMER_MSEC, 1);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This reminds me of #17330

cpu/sam0_common/sam0_sdhc/sdhc.c Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Outdated Show resolved Hide resolved
cpu/sam0_common/sam0_sdhc/sdhc.c Show resolved Hide resolved
@benpicco benpicco added State: waiting for author State: Action by the author of the PR is required and removed State: waiting for author State: Action by the author of the PR is required CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 24, 2022
maribu
maribu approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Code looks good and I trust your testing

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2022
@benpicco benpicco merged commit 4b6da5c into RIOT-OS:master May 31, 2022
@benpicco
Copy link
Contributor Author

Thank you for the review!

@benpicco benpicco deleted the sam0_sdhc branch May 31, 2022 13:33
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

@maribu maribu maribu approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@keestux keestux Awaiting requested review from keestux keestux is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @maribu @dylad @chrysn