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

drivers/mtd_sdmmc: always enable the erase function #20180

Merged
merged 5 commits into from
Dec 14, 2023
Merged

drivers/mtd_sdmmc: always enable the erase function #20180

merged 5 commits into from
Dec 14, 2023

Conversation

benpicco
Copy link
Contributor

Contribution description

For some reason the .erase_sector is a no-op by default when there is a perfectly cromulent implementation available.

Always enable it to comply with the MTD API.

Testing procedure

I run tests/drivers/mtd_raw on same54-xpro

master

2023-12-13 17:00:25,934 - INFO # Manual MTD test
2023-12-13 17:00:25,937 - INFO # init MTD_0… OK (8 MiB)
2023-12-13 17:00:25,940 - INFO # init MTD_1… OK (256 byte)
2023-12-13 17:00:26,150 - INFO # init MTD_2… OK (15223 MiB)

2023-12-13 17:00:41,721 - INFO # > test 2
2023-12-13 17:00:41,722 - INFO # 
2023-12-13 17:00:41,726 - INFO # [START]
2023-12-13 17:00:41,862 - INFO # tests/drivers/mtd_raw/main.c:382 => failed condition

this PR

2023-12-13 17:07:07,125 - INFO # Manual MTD test
2023-12-13 17:07:07,128 - INFO # init MTD_0… OK (8 MiB)
2023-12-13 17:07:07,131 - INFO # init MTD_1… OK (256 byte)
2023-12-13 17:07:07,342 - INFO # init MTD_2… OK (15223 MiB)

2023-12-13 17:07:16,275 - INFO # > test 2
2023-12-13 17:07:16,275 - INFO # 
2023-12-13 17:07:16,275 - INFO # [START]
2023-12-13 17:07:16,963 - INFO # [SUCCESS]

Issues/PRs references

@benpicco benpicco requested a review from gschorcht December 13, 2023 16:12
@github-actions github-actions bot added the Area: drivers Area: Device drivers label Dec 13, 2023
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2023
@benpicco benpicco mentioned this pull request Dec 13, 2023
Merged
5 tasks
@riot-ci
Copy link

riot-ci commented Dec 13, 2023 •
edited
Loading

Murdock results

✔️ PASSED

5e30c0e pkg/lwext4: make use of mtd_write_sector()

Success Failures Total Runtime
8082 0 8082 09m:58s

Artifacts

@gschorcht
Copy link
Contributor

gschorcht commented Dec 13, 2023 •
edited
Loading

SD memory cards and MMC/eMMCs should handle sector erase internally when writing, so it should not be necessary to explicitly erase sectors before writing to them here:

RIOT/drivers/mtd/mtd.c

Line 233 in bee6c17

res = mtd_erase_sector(mtd, sector, 1);
For this reason, MTD_SDMMC_ERASE was defined in Kconfig and disabled by default.

gschorcht
gschorcht reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

This makes

RIOT/drivers/mtd_sdmmc/Kconfig

Line 26 in bee6c17

config MTD_SDMMC_ERASE
obsolete.

@gschorcht gschorcht mentioned this pull request Dec 13, 2023
Merged
@benpicco
Copy link
Contributor Author

Ah but I think we should just set MTD_DRIVER_FLAG_DIRECT_WRITE and check for the flag in fatfs/lwext4 so we don't need to erase the page first.

benpicco
benpicco commented Dec 13, 2023
View reviewed changes
drivers/mtd_sdmmc/mtd_sdmmc.c
Comment on lines -154 to -162
memset(dev->work_area, 0, SDMMC_SDHC_BLOCK_SIZE);
while (count) {
if (sdmmc_write_blocks(mtd_sd->sdmmc, sector, SDMMC_SDHC_BLOCK_SIZE,
1, dev->work_area, NULL)) {
return -EIO;
}
--count;
++sector;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I don't understand is why you added this fallback implementation when you have a proper sdmmc_erase_blocks().

This was only needed for sdcard_spi because it does not implement the erase command at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I don't understand is why you added this fallback implementation when you have a proper sdmmc_erase_blocks().

To be honest, I don't remember. Probably, I just used mtd_sdcard_spi as a template to implement mtd_sdmmc so that it is just a copy of it. I didn't know the MTD_DRIVER_FLAG_DIRECT_WRITE flag.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Dec 13, 2023
gschorcht
gschorcht reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Please remove the

RIOT/drivers/mtd_sdmmc/Kconfig

Line 26 in bee6c17

config MTD_SDMMC_ERASE
in Kconfig.

@gschorcht
Copy link
Contributor

Please remove the

RIOT/drivers/mtd_sdmmc/Kconfig

Line 26 in bee6c17

config MTD_SDMMC_ERASE

in Kconfig.

RIOT/drivers/include/mtd_sdmmc.h

Lines 48 to 68 in a4fcf21

/**
* @defgroup drivers_mtd_sdmmc_config SD Memory Card driver compile configuration
* @ingroup config_drivers_storage
* @{
*/
/**
* @brief Enable SD Memory Card Erase
*
* SD Memory Cards and MMCs/eMMCs handle sector erase internally
* so it's possible to directly write to the card without erasing
* the sector first.
*
* @note An erase call will NOT touch the content if `CONFIG_MTD_SDMMC_ERASE`
* is not set, so enable this feature to ensure overriding the data.
*
* @pre This feature requires the `mtd_write_page` module.
*/
#ifdef DOXYGEN
#define CONFIG_MTD_SDMMC_ERASE
#endif
/** @} */

should be removed too.

@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Dec 14, 2023
gschorcht
gschorcht approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Please squash.

drivers/include/mtd.h Outdated Show resolved Hide resolved
@benpicco benpicco enabled auto-merge December 14, 2023 13:42
@benpicco benpicco added this pull request to the merge queue Dec 14, 2023
Merged via the queue into RIOT-OS:master with commit d13e6c4 Dec 14, 2023
25 checks passed
@benpicco benpicco deleted the mtd_sdmmc-erase branch December 14, 2023 16:52
@gschorcht gschorcht mentioned this pull request Jan 7, 2024
Merged
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht approved these changes

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

@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: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @riot-ci @gschorcht @MrKevinWeiss