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

Stm32f3: add flash support #2083

Merged
merged 24 commits into from Sep 17, 2020
Merged

Stm32f3: add flash support #2083

merged 24 commits into from Sep 17, 2020

Conversation

krady21
Copy link
Contributor

@krady21 krady21 commented Aug 24, 2020 •
edited

Pull Request Overview

This pull request adds flash memory support for stm32f303xc (embedded flash and option bytes).

Testing Strategy

This pull request was tested using a stm32f3discovery kit. All the main operations have been tested and work properly (read, write, erase).

TODO or Help Wanted

I am still somewhat unsure about how to treat the callbacks from writing and erasing the option bytes.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@krady21
add register struct for flash controller
8b833af
@krady21
Added Flash base address, OptionByte register and implemented part of
fd64f8f 
the hil::flash::Flash trait and a generic struct for Stm32f303 Flash
page.
@krady21
Use crate::flash in chip.rs
a0e6521
@krady21
Add erase_page, erase_all and write_page implementations
a7a915c
@krady21
Added read_page function
0ab0030
@krady21
Fix byte iteration
bed050d
@krady21
Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash
9973e12
@krady21
Fix return type of is_locked
b768af7
@krady21
Add crate definition in lib.rs and handle_interrupt function in flash.rs
6e7dce3
@krady21
Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash
39a90e1
@krady21
Use hardware interrupts for write and erase instead of deferred calls.
81389a3 
Implement write and erase for the option bytes. Cleanup the comments.
@krady21
Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash
cd5f2a2
@krady21
Fix comments and run make prepush
fec7459
@krady21 krady21 changed the title Stm32f3: Add flash support Stm32f3: add flash support Aug 24, 2020
hudson-ayers
hudson-ayers requested changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! I am excited to have flash support for an STM chip!

I have not completely reviewed this yet, but I left a few comments inline. One other comment: generally, we want upstream Tock boards to include support for all chip peripherals. This makes release testing much easier, and ensures that driver code continues to compile as Tock changes. Could you add the virtual flash to the stm32f3discovery board (using this driver) as part of this PR?

chips/stm32f303xc/src/flash.rs Outdated Show resolved Hide resolved
chips/stm32f303xc/src/flash.rs Outdated Show resolved Hide resolved
chips/stm32f303xc/src/flash.rs Outdated Show resolved Hide resolved
chips/stm32f303xc/src/flash.rs Outdated Show resolved Hide resolved
@krady21
Copy link
Contributor Author

krady21 commented Aug 24, 2020 •
edited

Could you give me some pointers in regards to adding the virtual flash? Do i have to write an additional flash capsule that I would use alongside the virtual flash capsule similar to what is done with the Alarm one?

@hudson-ayers
Copy link
Contributor

Could you give me some pointers in regards to adding the virtual flash? Do i have to write an additional flash capsule that I would use alongside the virtual flash capsule similar to what is done with the Alarm one?

Sorry, I shouldn't have said virtual flash in my comment -- I just meant that there should be some capsule using the driver, instantiated in boards/stm32f3discovery/src/main.rs. Implementing the nonvolatile storage driver should be fine. There are examples of how to do so in https://github.com/tock/tock/blob/master/boards/imix/src/main.rs and https://github.com/tock/tock/blob/master/boards/nordic/nrf52840dk/src/main.rs

@krady21
Copy link
Contributor Author

krady21 commented Aug 24, 2020 •
edited

Could you give me some pointers in regards to adding the virtual flash? Do i have to write an additional flash capsule that I would use alongside the virtual flash capsule similar to what is done with the Alarm one?

Sorry, I shouldn't have said virtual flash in my comment -- I just meant that there should be some capsule using the driver, instantiated in boards/stm32f3discovery/src/main.rs. Implementing the nonvolatile storage driver should be fine. There are examples of how to do so in https://github.com/tock/tock/blob/master/boards/imix/src/main.rs and https://github.com/tock/tock/blob/master/boards/nordic/nrf52840dk/src/main.rs

Ok. I'm on it!

hudson-ayers
hudson-ayers previously approved these changes Aug 24, 2020
View reviewed changes
@krady21
Copy link
Contributor Author

krady21 commented Aug 25, 2020 •
edited

I have one question regarding the userspace address and length used by NonvolatileStorageComponent::new(). In the last commit, I used the ones that I found in chip_layout.ld. Can you confirm that they are the right ones? I am worried that i might overwrite the code segment.

alistair23
alistair23 previously approved these changes Aug 25, 2020
View reviewed changes
@hudson-ayers
Copy link
Contributor

I have one question regarding the userspace addresses and length used by NonvolatileStorageComponent::new(). In the last commit, I used the ones that I found in chip_layout.ld. Can you confirm that they are the right ones? I am worried that i might overwrite the code segment.

I am actually not sure how the userspace addresses/length are supposed to be chosen. Neither Imix nor the nrf52840dk assign the entire prog region as userspace accessible though, which makes me nervous that this does... @bradjc ?

@krady21
change addresses used by nonvolatile component
0d018ba 
Used sstorage and estorage for the kernel address and length,
as suggested. Changed the userspace address and length to
0x08038000 and 0x2000 in src/main.rs and chip_layout.ld to
avoid overwriting the code segment
@krady21 krady21 dismissed stale reviews from alistair23 and hudson-ayers via 0d018ba August 26, 2020 13:44
@krady21
Copy link
Contributor Author

krady21 commented Aug 27, 2020 •
edited

Ended up choosing 0x08038000 and 0x8000 (32KB) for the userspace address and length (prog being between 0x08020000 and 0x08040000). This leaves us with 96KB for the application binaries and 16 pages available for userspace programming. Imix does a similar thing, using 0x60000, while its prog area is between 0x40000 and 0x80000. Let me know if you have a different split in mind and sorry for having you review this twice.

hudson-ayers
hudson-ayers previously approved these changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

I am happy with this PR, but would like if someone more familiar than me with how we assign userspace flash regions could double check that.

alistair23
alistair23 previously approved these changes Aug 28, 2020
View reviewed changes
alistair23
alistair23 previously approved these changes Sep 11, 2020
View reviewed changes
ppannuto
ppannuto reviewed Sep 11, 2020
View reviewed changes
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

I think this looks good -- one place where some comments would be valuable, but otherwise looks good to me

boards/stm32f3discovery/src/main.rs Outdated Show resolved Hide resolved
@ppannuto ppannuto added the last-call Final review period for a pull request. label Sep 11, 2020
@bradjc
Copy link
Contributor

bradjc commented Sep 17, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 17, 2020

Build succeeded:

@bors bors bot merged commit 14dda51 into tock:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppannuto ppannuto ppannuto approved these changes

@bradjc bradjc bradjc approved these changes

@hudson-ayers hudson-ayers Awaiting requested review from hudson-ayers

@alistair23 alistair23 Awaiting requested review from alistair23

Assignees
No one assigned
Labels
last-call Final review period for a pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@krady21 @hudson-ayers @bradjc @alistair23 @ppannuto