Added flashing support and extended the flash module

[korken89]

I extended the flash support to be able to flash the internal flash.
It works fine, but feedback in welcome!

[korken89]

Example code for usage:

    let mut flash = dp.FLASH.constrain();
    let mut rcc = dp.RCC.constrain();
    let mut pwr = dp.PWR.constrain(&mut rcc.apb1r1);

    let _clocks = rcc
        .cfgr
        .lse(CrystalBypass::Disable, ClockSecuritySystem::Disable)
        .msi(MsiFreq::RANGE4M)
        .sysclk(80.mhz())
        .freeze(&mut flash.acr, &mut pwr);

    let mut prog = flash.keyr.unlock_flash(&mut flash.sr, &mut flash.cr).unwrap();
    prog.erase_page(FlashPage(4)).unwrap()
    prog.program_dword(FlashPage(4).to_address(), 0x0123_4567_89ab_cdef).unwrap();
    assert!(core::ptr::read(FlashPage(4).to_address() as *const u64) == 0x0123_4567_89ab_cdef);

[MabezDev]

Looking good! Are you planning on resolving the TODO in this PR? Or is that future work?

[korken89]

Thanks for the feedback!

On the TODO, I'm having a look at it now.
This is the function that should be used to minimize flash wear but limits one to only flash 32 u64 chunks at a time, I want to see if one can alleviate this requirement.

[korken89]

I had a look at the fast programming, this is only valid after a mass erase, i.e. only for a program running in RAM. I did not have plans for testing RAM based programs now (though the interface does support it through the normal programming), so I'll leave the fast_programming out for now and maybe revisit that specific feature in the future.

Also I moved to a slice based interface as enabling and disabling the flashing interface made it quite slow.
Plus added example documentation to the flash module.

I think this is ready for review now.

[korken89]

New example:

fn program_region(mut flash: flash::Parts) -> Result<(), flash::FlashError> {
    // Unlock the flashing module
    let mut prog = flash.keyr.unlock_flash(&mut flash.sr, &mut flash.cr)?;

    let page = flash::FlashPage(5);

    // Perform the erase and programing operation
    prog.erase_page(page)?;
    let data = [
        0x1111_1112_1113_1114,
        0x2221_2222_2223_2224,
        0x3331_3332_3333_3334,
    ];
    prog.program(page.to_address(), &data)?;

    // Check result (not needed, but done for this example)
    let addr = page.to_address() as *const u64;
    assert!(unsafe { core::ptr::read(addr) } == data[0]);
    assert!(unsafe { core::ptr::read(addr.offset(1)) } == data[1]);
    assert!(unsafe { core::ptr::read(addr.offset(2)) } == data[2]);

    Ok(())
}

[nickray]

Are you thinking about actual embedded-hal flash traits? cf. rust-embedded/embedded-hal#28

[korken89]

@nickray Yeah, I had a look in embedded-hal but none existed.
The ones you have linked seem to be in a WIP/experimental stage, do you want feedback on it?

Example:

fn write(&mut self, address: usize, data: &[u8]) -> FlashResult {
    assert!(data.len() % WriteSize::to_usize() == 0);
    assert!(address % WriteSize::to_usize() == 0);

    // interrupt::free(|cs| {
        for i in (0..data.len()).step_by(8) {
            self.write_native(
                address + i,
                GenericArray::from_slice(&data[i..i + 8]),
                // cs,
                )?;
        }
        Ok(())
    // })
}

Assumes 8 bytes, but also has as WriteSize to determine the size. I think this is a bug in the trait, but not sure if there are other thoughts behind it.

[korken89]

@nickray I see in the documentation that you were thinking about how to get compile time alignment and data size correct. I would move to use the native data types in write_native for this, i.e u8 if byte aligned, u16 if half-word aligned, u32 if word aligned, u64 if double-word aligned, and u128 if quad-word aligned.
With that you will get it for free rather than forcing the &[u8] interface.

I can make an example trait of this if you'd like.

[nickray]

First one is a bug... Also if this is too off-topic, happy to discuss more in an lpc55-hal issue.

Latest iteration is https://github.com/nickray/lpc55-hal/blob/main/src/traits/flash.rs, definitely interested in feedback! I think that is what you meant by adjusting write_native? Alignment is missing, fizyk20/generic-array#86 doesn't work directly but should be doable. Most asserts should probably be debug asserts.

My main/only use case currently is implementing something like https://github.com/nickray/littlefs2/blob/main/src/driver.rs. (On that topic, there is some shared interest with other projects than SoloKeys, e.g. iqlusioninc/usbarmory.rs#39 plan (I think...) to do a rewrite of littlefs in Rust, which would profit from "official"/shared flash traits).

I dropped the "unlock guard" for now, not sure if that should be modeled with typestates (giving the user responsibility to lock), a guard (but what if that panics), or with a closure perhaps. If only trusted code has access to the flash (I'm using it for a "crypto service" in TrustZone), then perhaps the trait shouldn't feel responsible for "lock safety".

[korken89]

Look a lot better in the latest version. What is the issue with using the Aligned wrapper? Maybe too much noise on the user side?

[nickray]

I don't quite remember unfortunately. If you're interested in collaborating on some traits, let me know. We'd then already start out with "two implementations" :)

[korken89]

Sure, then we have the 2 needed implementations for HAL inclusion!

[MathiasKoch]

I think this looks good.
@MabezDev Any reason not to merge?

[korken89]

Maybe merge as is, while @nickray and I can make a new PR using a trait?

@nickray any objections to this?

[korken89]

I should have a bit of time over towards the end of today, I'll give the flash trait a go to see if we can use it directly.

[nickray]

I imagine implementing the eventual flash trait would be supplemented by device specific HAL functions anyway. So I'm +1 for merging this as-is for now.

The flash trait should go through an RFC process anyway I assume (does it implement any length read from any address? is pulling in generic-array acceptable? are we focused only on on-chip flash or e.g. SPI flash? etc. etc.), so I think that will be a week or two at least.

[korken89]

@nickray I made a minor change to your trait to get memory alignment and better freedom for the implementer. I pushed the change in my latest commit.

I removed all references to GenericArray and such, what do you think?

[korken89]

I forgot to write what I did...
I moved to an associated type instead of GenericArray, which only has the requirement to be of correct alignment (as long as the implementation respects the underlying representation).
This gives greater freedom to the implementer which can, if they want, use GenericArray to make the implementation or any other type with correct alignment.

[korken89]

@nickray I have implemented the read and write trait now as well. You can have a look and give feedback :)

[korken89]

@nickray Any feedback? Else I'll opt for merging this as is.

[nickray]

Ah sorry, this fell off my radar, thanks for pinging me.

I was thinking/hoping the slice interface would be independent of the native implementation. Is this not possible here with your approach (meaning the trait has a default implementation of slice in terms of native)?

[korken89]

Not sure how to do taht as we have multiple alignments to take into consideration. Plus the type need to implement the *_ne_bytes that does not exist as a trait.
Do you have an idea of how to do it?

[nickray]

What do you mean by multiple alignments? I don't know about flash enough, but I'd assume read/write/page size coincides with their alignments, so one could use that. For the other, I think what you need is to Deref (maybe?) to byte slices?

To me the whole slice/native split (with encoding the native interface in the trait) doesn't make much sense if each HAL has to do everything by hand (basically if you can't implement the slice API generically via the native API, how's a potential driver going to use it, or what for?).

In my old version with generic-array there was enough concreteness in an underlying slice type to implement this, again, not sure if/how to do it with your approach.

[korken89]

The issue does not lay in the write_native, the code becomes as it did because address and the data may not aligned to the native writes.

In your original code this is checked by

assert!(data.len() % WriteSize::to_usize() == 0);
assert!(address % WriteSize::to_usize() == 0);

However this can make user applications start failing without compile fail when moved between MCUs. As this alignment of the write function cannot be checked at compile time (without always using the native interface, but then the interop is gone anyways).

As I see it, you will need MCU specific adaptations of the write method to not get into these issues and a generic implementation is difficult.
I am very open to suggestions of how to do that though. One way is to do a mix, use the aligned generic array for the native implementation, and then a generic write implementation.
But I am unsure on how to do the start and end alignment with generic array.

[korken89]

Ping @nickray

If we cannot get a consensus now I suggest we merge this as is and continue the trait development in a separate PR.

[nickray]

Yeah absolutely!

[korken89]

Do you have anything you want to add @MabezDev else I propose we merge :)

[MabezDev]

LGTM.