Stm32f3: add flash support by krady21 · Pull Request #2083 · tock/tock

krady21 · 2020-08-24T12:38:08Z

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.

the hil::flash::Flash trait and a generic struct for Stm32f303 Flash page.

Implement write and erase for the option bytes. Cleanup the comments.

hudson-ayers

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?

Co-authored-by: Hudson Ayers <32688905+hudson-ayers@users.noreply.github.com>

2. Added callbacks to PGERR and WRPRTERR

…stm32f3-flash

krady21 · 2020-08-24T17:59:03Z

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 · 2020-08-24T18:04:22Z

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 · 2020-08-24T18:05:50Z

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!

krady21 · 2020-08-25T09:34:57Z

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.

hudson-ayers · 2020-08-26T03:48:27Z

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 ?

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 · 2020-08-27T18:44:09Z

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

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.

ppannuto

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

bradjc · 2020-09-17T14:48:34Z

bors r+

bors · 2020-09-17T15:03:59Z

Build succeeded:

2083: Stm32f3: add flash support r=bradjc a=krady21 ### 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 - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. 2101: RFC: Remove `AppPtr::Drop()` and `Owned::Drop()` r=bradjc a=hudson-ayers ### Pull Request Overview This PR proposes removing `AppPtr::Drop()` and `Owned::Drop()`, as well as the `ProcessType::free()` method that they each call. Currently, `Process::free()` is implemented with an empty body, merely dropping the passed raw pointer. Therefore, as far as I can tell, calls to `Drop()` on each type have no useful effect. Despite this, the presence of these implementations have a significant cost -- the implementations of `Drop` call `kernel.process_map_or()`, which takes a trait object reference (e.g. `&dyn ProcessType`), so the compiler cannot know that the concrete `free()` function that will be called is empty, and optimize the call out. Currently, anytime an `AppPtr` or `Owned` type goes out of scope -- including anytime a struct containing such a type goes out of scope -- `drop()` is called on each type until the underlying `drop()` implementation is called. This happens frequently: a single iteration of the loop in the `Imix` app calls `AppPtr::drop()` 45 times; a single iteration of the `hello_loop` app calls `AppPtr::drop()` 4 times and `Owned::drop()` 3 times. Further, each call has some cost: ##### Cycles per call, measured using cortex-m DWT cycle counter peripheral: `AppSlice::drop()` - 139 cycles (increases to 145 if the board is configured for 8 processes instead of 4) `Owned::drop()` - 84-150 cycles, depending on context. Removing the Drop implementations for `AppPtr` and `Owned` reduces the measured cycle count in each case to 1 cycle. `AppSlice::drop()` is called regularly (inserted by the compiler) within the body of `allow()` system call implementations, `Owned::drop()` is called within the body of `Grant::enter()` and `Grant::each()`. Keeping in mind recent attempts to reduce Tock syscall overhead, I think that we should not pay all of this overhead for no benefit as Tock is implemented today. Alternatively, if there is reason to believe these implementations will be useful in the future, I think we should document these functions (and `ProcessType::free()`) as to why they are present. ### Testing Strategy This pull request was tested by running several apps with the drop implementations removed. ### TODO or Help Wanted The Tock SOSP paper talks a bit about the `Owned::drop()` implementation, but as far as I can tell it does not actually do anything in Tock today. Is there reason to believe we will need these implementations for some future process type? Am I totally missing something here? ### Documentation Updated - [x] No updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Bogdan Grigoruta <bogdangrigoruta@gmail.com> Co-authored-by: Bogdan Grigoruta <43993819+krady21@users.noreply.github.com> Co-authored-by: Hudson Ayers <hayers@stanford.edu>

krady21 added 13 commits July 13, 2020 20:53

add register struct for flash controller

8b833af

Added Flash base address, OptionByte register and implemented part of

fd64f8f

the hil::flash::Flash trait and a generic struct for Stm32f303 Flash page.

Use crate::flash in chip.rs

a0e6521

Add erase_page, erase_all and write_page implementations

a7a915c

Added read_page function

0ab0030

Fix byte iteration

bed050d

Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash

9973e12

Fix return type of is_locked

b768af7

Add crate definition in lib.rs and handle_interrupt function in flash.rs

6e7dce3

Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash

39a90e1

Use hardware interrupts for write and erase instead of deferred calls.

81389a3

Implement write and erase for the option bytes. Cleanup the comments.

Merge branch 'master' of https://github.com/tock/tock into stm32f3-flash

cd5f2a2

Fix comments and run make prepush

fec7459

krady21 changed the title ~~Stm32f3: Add flash support~~ Stm32f3: add flash support Aug 24, 2020

hudson-ayers requested changes Aug 24, 2020

View reviewed changes

Comment thread chips/stm32f303xc/src/flash.rs Outdated

Comment thread chips/stm32f303xc/src/flash.rs Outdated

Comment thread chips/stm32f303xc/src/flash.rs Outdated

Comment thread chips/stm32f303xc/src/flash.rs Outdated

krady21 and others added 4 commits August 24, 2020 17:29

Update chips/stm32f303xc/src/flash.rs

aa7916f

Co-authored-by: Hudson Ayers <32688905+hudson-ayers@users.noreply.github.com>

1. Changed as some comments as requested.

00d5d4a

2. Added callbacks to PGERR and WRPRTERR

Chang some comments as requested and add callbacks to PGERR and WRPRTERR

0b3ca18

Merge branch 'stm32f3-flash' of https://github.com/krady21/tock into …

183b8b5

…stm32f3-flash

hudson-ayers reviewed Aug 24, 2020

View reviewed changes

Comment thread chips/stm32f303xc/src/flash.rs Outdated

Remove last while loop waiting on the BSY bit

479b5ae

add nonvolatile_storage capsule using the stm32 flash driver

a996d10

hudson-ayers previously approved these changes Aug 24, 2020

View reviewed changes

alistair23 previously approved these changes Aug 25, 2020

View reviewed changes

hudson-ayers reviewed Aug 26, 2020

View reviewed changes

Comment thread boards/stm32f3discovery/src/main.rs Outdated

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 dismissed stale reviews from alistair23 and hudson-ayers via 0d018ba August 26, 2020 13:44

fix whitespace

82a983a

krady21 requested review from alistair23 and hudson-ayers August 27, 2020 07:33

change wrong userspace length

1b7e4e0

hudson-ayers previously approved these changes Aug 28, 2020

View reviewed changes

alistair23 previously approved these changes Aug 28, 2020

View reviewed changes

krady21 commented Sep 11, 2020

View reviewed changes

Comment thread chips/stm32f303xc/src/flash.rs

move mod flash to peripherals section

319d2f6

krady21 dismissed stale reviews from alistair23 and hudson-ayers via 319d2f6 September 11, 2020 14:27

krady21 requested review from alistair23 and hudson-ayers September 11, 2020 14:27

alistair23 previously approved these changes Sep 11, 2020

View reviewed changes

ppannuto reviewed Sep 11, 2020

View reviewed changes

Comment thread boards/stm32f3discovery/src/main.rs Outdated

ppannuto added the last-call Final review period for a pull request. label Sep 11, 2020

add comments and change wrong page number

e131a13

krady21 dismissed alistair23’s stale review via e131a13 September 11, 2020 17:55

krady21 requested review from alistair23 and ppannuto September 11, 2020 17:56

ppannuto approved these changes Sep 11, 2020

View reviewed changes

bradjc approved these changes Sep 17, 2020

View reviewed changes

bors Bot merged commit 14dda51 into tock:master Sep 17, 2020

Uh oh!

Conversation

krady21 commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

hudson-ayers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krady21 commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hudson-ayers commented Aug 24, 2020

Uh oh!

krady21 commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krady21 commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hudson-ayers commented Aug 26, 2020

Uh oh!

krady21 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hudson-ayers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ppannuto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradjc commented Sep 17, 2020

Uh oh!

bors Bot commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krady21 commented Aug 24, 2020 •

edited

Loading

krady21 commented Aug 24, 2020 •

edited

Loading

krady21 commented Aug 24, 2020 •

edited

Loading

krady21 commented Aug 25, 2020 •

edited

Loading

krady21 commented Aug 27, 2020 •

edited

Loading