Skip to content

mtd: refactor mtd struct for better erase support#8411

Draft
vincent-d wants to merge 10 commits intoRIOT-OS:masterfrom
OTAkeys:pr/refactor_mtd
Draft

mtd: refactor mtd struct for better erase support#8411
vincent-d wants to merge 10 commits intoRIOT-OS:masterfrom
OTAkeys:pr/refactor_mtd

Conversation

@vincent-d
Copy link
Member

@vincent-d vincent-d commented Jan 23, 2018

Contribution description

As said in #8400, erase in mtd spi nor is not working correctly as on some flash, the erase size might be smaller than the sector size.

Thie PR adds min_erase_size and sector_size fields in the mtd_dev_t to fix that.

On spi flashes supporting different erase size, the config can be (for instance a flash with 64KB blocks and 4KB erasable sectors):
sector_size = 65536
min_erase_size = 4096

Issues/PRs references

This is based on #8399 and #8400

@vincent-d vincent-d added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 23, 2018
@vincent-d vincent-d requested a review from jnohlgard January 23, 2018 11:47
@vincent-d vincent-d mentioned this pull request Feb 8, 2018
Merged
@vincent-d vincent-d added the Area: fs Area: File systems label Feb 20, 2018
@vincent-d vincent-d removed the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 26, 2018
@vincent-d
Copy link
Member Author

vincent-d commented Feb 26, 2018

Rebased since #8399 and #8400 have been merged

@vincent-d
Copy link
Member Author

vincent-d commented Mar 5, 2018

Rebased

Vincent Dupont added 4 commits May 14, 2018 15:08
mtd: refactor mtd struct
00dd963 
Add a min_erase_size field and sector_size and remove pages_per_sector
field. This allow to use a different erase size than the sector size.
mtd_native: update implementation
bcb7bce
mtd_spi_nor: update implementation
dac43c1
mtd_vfs: update implementation
8afef56
@vincent-d
Copy link
Member Author

vincent-d commented May 14, 2018

Rebased.

Could someone take a look at this one?

@tcschmidt
Copy link
Member

tcschmidt commented May 25, 2018

@gebart can you review or shall we look for someone new?

@tcschmidt
Copy link
Member

tcschmidt commented May 25, 2018

@gebart can you review or shall we look for a new reviewer?

jnohlgard
jnohlgard requested changes Sep 11, 2018
View reviewed changes
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

A useful improvement when using memories which support smaller erase sizes.
The changes to the mtd driver implementations look OK. The FS pkg changes look OK, but it would be good if someone with better knowledge of the fatfs and littlefs drivers would take a look as well.
See inline comment on a possible typo in the tests.
Good to see the test mock driver updated with flash AND behavior 👍

tests/unittests/tests-littlefs/tests-littlefs.c
#ifndef PAGE_PER_SECTOR
#define PAGE_PER_SECTOR 4
#ifndef SECTOR_SIZE
#define SECTOR_SIZE 128
Copy link
Member

@jnohlgard jnohlgard Sep 11, 2018

Choose a reason for hiding this comment

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

Is this a typo or was it intentional?
4*64 = 256

Copy link
Member Author

@vincent-d vincent-d Sep 11, 2018

Choose a reason for hiding this comment

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

I think it was intentional to reduce the RAM usage and allow testing on more boards. It's been quite some time since I wrote this ;)

Copy link
Member

@smlng smlng Jan 7, 2019

Choose a reason for hiding this comment

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

IIRC @vincent-d is right, we reduced that some time ago to allow for testing on more boards ... specifically some smaller nucleo32s, reducing it even further can only help.

@aabadie aabadie added this to the Release 2019.01 milestone Jan 4, 2019
@aabadie
Copy link
Contributor

aabadie commented Jan 4, 2019

It seems we are not that far from merging this PR.
Maybe @smlng can help reviewing and testing this PR since he reviewed #8239 (littlefs). For FatFS, let's also ping @MichelRottleuthner and again @smlng: they were both involved in #7104 and #6072.

@tcschmidt
Copy link
Member

tcschmidt commented Jan 4, 2019

Maybe also @x3ro can easily help here?

@x3ro
Copy link
Contributor

x3ro commented Jan 4, 2019

Interesting, I'll see if I can find time to look at this during the next week.

@x3ro
Copy link
Contributor

x3ro commented Jan 8, 2019

Hmm, I understand the need for min_erase_size but can't seem to figure out why it was necessary to replace pages_per_sector with sector_size. I feel like I'm missing something obvious. Can you help out, @vincent-d?

@stale
Copy link

stale bot commented Jan 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 17, 2020
@bergzand bergzand added the State: don't stale State: Tell state-bot to ignore this issue label Feb 13, 2020
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Feb 13, 2020
@bergzand
Copy link
Member

bergzand commented Feb 13, 2020

@vincent-d Could you give this a rebase? I can give this a review one of these days.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@mguetschow
Copy link
Contributor

mguetschow commented Jun 11, 2024

Since this PR has been stale for several years, I'll convert it to a draft.

Please feel free to remove the draft state if anyone wants to pick this up again.

@mguetschow mguetschow marked this pull request as draft June 11, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard requested changes

@miri64 miri64 Awaiting requested review from miri64 miri64 will be requested when the pull request is marked ready for review miri64 is a code owner

@Teufelchen1 Teufelchen1 Awaiting requested review from Teufelchen1 Teufelchen1 will be requested when the pull request is marked ready for review Teufelchen1 is a code owner

+1 more reviewer

@smlng smlng smlng left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Area: fs Area: File systems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: don't stale State: Tell state-bot to ignore this issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@vincent-d @tcschmidt @aabadie @x3ro @bergzand @mguetschow @jnohlgard @smlng @miri64 @MrKevinWeiss