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_flashpage: improve _write_page #20173

Merged
merged 1 commit into from
Jan 2, 2024
Merged

drivers/mtd_flashpage: improve _write_page #20173

merged 1 commit into from
Jan 2, 2024

Conversation

fabian18
Copy link
Contributor

Contribution description

Some time ago I noticed that the mtd_flashpage driver was not writing the size of a full block (FLASHPAGE_WRITE_BLOCK_SIZE) when it could be, on unaligned access.
It was only writing FLASHPAGE_WRITE_BLOCK_ALIGNMENT.
So this PR is just a tiny performance improvement.

Testing procedure

That case even happens in the test tests/drivers/mtd_flashpage.
And with his change it still passes on same54-xpro.

2023-12-13 00:07:20,959 # START
2023-12-13 00:07:20,967 # main(): This is RIOT! (Version: 2024.01-devel-360-ga5be0-pr/mtd_flashpage_improve_write_page)
2023-12-13 00:07:22,206 # .....
2023-12-13 00:07:22,207 # OK (5 tests)
2023-12-13 00:07:22,213 # { "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 448 }]}

Issues/PRs references

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Dec 12, 2023
@fabian18 fabian18 added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Dec 12, 2023
benpicco
benpicco reviewed Dec 14, 2023
View reviewed changes
drivers/mtd_flashpage/mtd_flashpage.c Outdated
@@ -114,9 +114,13 @@ static int _write_page(mtd_dev_t *dev, const void *buf, uint32_t page, uint32_t
__attribute__ ((aligned (FLASHPAGE_WRITE_BLOCK_ALIGNMENT)));

offset = addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT;
size = MIN(size, FLASHPAGE_WRITE_BLOCK_ALIGNMENT - offset);
size = MIN(size, FLASHPAGE_WRITE_BLOCK_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not

Suggested change
size = MIN(size, FLASHPAGE_WRITE_BLOCK_SIZE);
size = MIN(size, FLASHPAGE_WRITE_BLOCK_SIZE - offset);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that it was wrong in some cases. I don´t remember in which cases. I will post an example later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I enabled the DEBUG() and tried an application with FLashDB. From the output it seems to be the same 🤔. I don´t know why I wrote it like this back then.

benpicco
benpicco reviewed Dec 18, 2023
View reviewed changes
drivers/mtd_flashpage/mtd_flashpage.c Outdated

DEBUG("flashpage: write %"PRIu32" unaligned bytes\n", size);
if (offset + size > FLASHPAGE_WRITE_BLOCK_SIZE) {
size = ((offset + size) & ~(FLASHPAGE_WRITE_BLOCK_SIZE - 1)) - offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment how you arrived at this formula?

@fabian18 fabian18 force-pushed the pr/mtd_flashpage_improve_write_page branch from a5be0a1 to 37c39d0 Compare December 30, 2023 23:37
@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 Jan 2, 2024
@riot-ci
Copy link

riot-ci commented Jan 2, 2024 •
edited
Loading

Murdock results

✔️ PASSED

37c39d0 drivers/mtd_flashpage: improve _write_page

Success Failures Total Runtime
8100 0 8100 10m:48s

Artifacts

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 2, 2024
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 2, 2024
@benpicco benpicco enabled auto-merge January 2, 2024 13:21
@benpicco benpicco added this pull request to the merge queue Jan 2, 2024
@github-merge-queue github-merge-queue bot merged commit ece8a12 into RIOT-OS:master Jan 2, 2024
32 checks passed
@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

@benpicco benpicco benpicco approved these changes

Assignees
No one assigned
Labels
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
6 participants
@fabian18 @riot-ci @benpicco @maribu @kaspar030 @MrKevinWeiss