Expose QSPI control register to switch used bank

[ElouanPetereau]

Hi, I'm using this library with a STM32 H7 series microcontroller which is connected to a NOR Flash with 2 chips.

Since this NOR Flash support QuadSPI, I wanted to use it but I also want to access to both memory chips.
Since Both of my flash chips are used separatly, one for image backup and one for temporary storage, I want to access to only one of them at a time.
My problem is that, when I looked at the Dual-Flash mode, it seems that I have to read and write to both chips at the same time as from the Quad-SPI interface on STM32 microcontrollers and microprocessors STM documentation, I can read:

Note that all bytes at even addresses are stored in Flash 1 while all bytes at odd addresses are stored in Flash 2.
As described in the figure above, in Dual-Flash mode the same command, address and alternate are sent to both Flash 1 and Flash 2.

This is really not ideal for me because I don't want to risk corrupting the backups when doing any operation on the other chip.

On top of that, I only have one clock and one reset GPIO line available for both since the chips have been mounted to use the Dual-Flash mode. This means that I can't create two QSPI peripherals using QUADSPI.bank1() and QUADSPI.bank2() since these functions consume the given pins and thus, the common clock.

This pull request is here to propose a solution for this case and for that I created a new QSPI.bank_switch() method that accept a PinsBank1And2 list of pins to consume and a default bank.
To change the addressed bank, I exposed the QUADSPI control register trhough a change_bank() method.

Also please note that I'm far from being an expert in Flash memories so if you know a solution to my problem that does not require any change in the STM32 hal I will gladly take it.

[richardeoin]

Thank you for the PR! Hopefully me or someone else will take a detailed look at it soon.

The continuous integration checks for nightly Rust are failing, but not because of your changes! So you don't need to fix anything.

[richardeoin]

This PR nicely adds the functionality you described. Thanks for that! I added some feedback in a comment.

It's worth noting that the change_bank method can be used no matter which of the bank1 bank2 bank_switch or qspi_unchecked methods was used to initialise the Qspi. This isn't necessarily a problem, but I think there's a clearer design possible:

• Rename change_bank to change_bank_unchecked to emphasise that the pins are not checked.
• Just use qspi_unchecked for initialisation, meaning the bank_switch initialiser is no longer required.

In any case the change_bank method is an import part of this API, so should have a doc comment explaining its function ("Switches between single-bank mode on Bank 1 and single-bank mode on Bank 2. Has no effect in dual-bank mode")

[richardeoin]

Bits FSEL and DFM can only be modified when the peripheral is not busy. The following check should be used to protect against that

// Wait for the peripheral to indicate it is no longer busy.
while self.is_busy().is_err() {}

It's already used several times in mod.rs. Instead of blocking you could also return an error from this function. Then the user would need to handle the case that a transaction is ongoing when change_bank is called.

[ElouanPetereau]

Thank you for your review.
I decided to return an error and let the user handle potential retry as I don't want to block the running code.

Let me know if the documentation comment that I added are enough or not.

Rename change_bank to change_bank_unchecked to emphasise that the pins are not checked.

I did that but since it's not a good thing to have the change_bank_unchecked for the struct generated by the other functions (bank1, bank2 and qspi_unchecked ) do you think it would be worth it to return an other type that have an inner Qspi<stm32::QUADSPI> or something like that?

Just use qspi_unchecked for initialisation, meaning the bank_switch initialiser is no longer required

I still think that it is better to consume the pins in this case no? Like it is done for the bank1 and bank2 functions.

[richardeoin]

The documentation is good and it's enough.

The change_bank_unchecked can only be restricted by adding a second generic type parameter to the Qspi type. That would then be a breaking change and perhaps too much complexity for solving this problem.

It's not ideal that the change_back_unchecked method is always available, but it's not a major problem either. Overall I'd be happy to accept the PR in the current state if you are also?

[ElouanPetereau]

The change_bank_unchecked can only be restricted by adding a second generic type parameter to the Qspi type. That would then be a breaking change and perhaps too much complexity for solving this problem.

A change that would not be API breaking would be to return a new Qspi type (QspiSwitchable for example) from bank_switch. This new type would have an inner Qspi exposed for any operation on classic Qspi and the function change_bank_unchecked so the normal Qspi won't have it.
However creating a type just for that might not be optimal.

I'd be happy to accept the PR in the current state if you are also?

If you are happy with the MR, and if you think that it is not mandatory to do any change to make the change_back_unchecked usable only when it actually does something , please do!

[ElouanPetereau]

Hello again,
Do you still plan to merge this PR or is there still something that I can do to improve it?

[richardeoin]

Hi! There was a period when v0.15.0 and v0.15.1 were being released that I avoided merging new changes, but now that's done. Thanks for the rebase!

I concluded that this PR is good to merge as-is, and experimenting with a slightly different way of implementing this (like the QspiSwitchable idea) can happen in another PR. So I'll merge this one now.