Skip to content

Spi memory lock #1415

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Spi memory lock #1415

wants to merge 1 commit into from

Conversation

Beerosagos
Copy link
Collaborator

@Beerosagos Beerosagos commented Apr 17, 2025 •
edited
Loading

This adds api calls to lock/unlock and write a protected area of the memory.

This PR includes a test function to be removed before merging

@Beerosagos Beerosagos force-pushed the spi-memory-lock branch 2 times, most recently from 28dc7c4 to 950a314 Compare April 17, 2025 10:56
Beerosagos
Beerosagos commented Apr 17, 2025
View reviewed changes
src/memory/spi_mem.c
// set the top/bottom protection bit.
// This is an OTP bit,so the write will have an effect
// only the first time.
reg[1] = reg[1] | CR1_TB_BIT_BOTTOM;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if it is ok to add this here, or if we want to do it inside the driver during the init function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as this gets executed during factor setup I think we are good.

@Beerosagos Beerosagos force-pushed the spi-memory-lock branch 2 times, most recently from 1ba0527 to 050dd3e Compare April 17, 2025 16:05
@Beerosagos Beerosagos force-pushed the spi-memory-lock branch 3 times, most recently from 49badb6 to b29fa51 Compare April 28, 2025 15:07
@Beerosagos Beerosagos marked this pull request as ready for review April 29, 2025 08:18
@benma benma requested a review from NickeZ May 14, 2025 09:26
@benma
Copy link
Collaborator

benma commented May 14, 2025

Assigned to @NickeZ who is more familiar with all this.

NickeZ
NickeZ reviewed May 14, 2025
View reviewed changes
Copy link
Collaborator

@NickeZ NickeZ left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general very good! Only some minor comments.

I also ran the test on my device which succeeded!

edit: I also unintendedly tested it because running the firmware in this PR left the memory locked, so flashing with the old code didn't work anymore 🎉

src/memory/spi_mem.c
// set the top/bottom protection bit.
// This is an OTP bit,so the write will have an effect
// only the first time.
reg[1] = reg[1] | CR1_TB_BIT_BOTTOM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as this gets executed during factor setup I think we are good.

@Beerosagos
memory/spi-mem: add spi memory api calls to protect the memory
22091b2 
This adds api calls to lock/unlock and write a protected area of the
memory.
@Beerosagos
Copy link
Collaborator Author

@NickeZ updated, PTAL 🙏

NickeZ
NickeZ approved these changes May 15, 2025
View reviewed changes
Copy link
Collaborator

@NickeZ NickeZ left a comment

Choose a reason for hiding this comment

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

ACK, I assume you will remove all changes from bootloader/startup.c and firmware.c before merging.

I just realized, does it make sense to move the test to factorysetup.c?

@Beerosagos
Copy link
Collaborator Author

ACK, I assume you will remove all changes from bootloader/startup.c and firmware.c before merging.

Sure, I guess we'll call spi_mem_protected_area_lock() somewhere in the factory setup code in a future PR, right?

I just realized, does it make sense to move the test to factorysetup.c?

I don't think so.. the test takes a lot of time to execute, I don't think we want that, wdyt?

@NickeZ
Copy link
Collaborator

NickeZ commented May 15, 2025

ACK, I assume you will remove all changes from bootloader/startup.c and firmware.c before merging.

Sure, I guess we'll call spi_mem_protected_area_lock() somewhere in the factory setup code in a future PR, right?

I'm pretty sure factory setup calls platform_init(). So as long as it is there we are good.

I just realized, does it make sense to move the test to factorysetup.c?

I don't think so.. the test takes a lot of time to execute, I don't think we want that, wdyt?

True, yeah, we want factory setup to be as minimal as possible. Since we write the ble firmware to the chip we know if it works during factory setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickeZ NickeZ NickeZ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Beerosagos @benma @NickeZ