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

Expose QSPI control register to switch used bank #449

Merged
merged 5 commits into from
Nov 16, 2023
Merged

Expose QSPI control register to switch used bank #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
028112e
Expose QSPI control register for the QSPI struct
ElouanPetereau Sep 1, 2023
157d028
Add enums and traits for Bank switch
ElouanPetereau Sep 1, 2023
de11d53
Expose BankSelect and related error
ElouanPetereau Sep 1, 2023
575a435
Rename QUADSPI change_bank and check for business
ElouanPetereau Oct 5, 2023
ba5e6de
Add documentation for Quad-SPI
ElouanPetereau Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 42 additions & 3 deletions src/xspi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@
mod qspi;
#[cfg(any(feature = "rm0433", feature = "rm0399"))]
pub use common::{
Bank, Xspi as Qspi, XspiError as QspiError, XspiMode as QspiMode,
XspiWord as QspiWord,
Bank, BankError, BankSelect, Xspi as Qspi, XspiError as QspiError,
XspiMode as QspiMode, XspiWord as QspiWord,
};
#[cfg(any(feature = "rm0433", feature = "rm0399"))]
pub use qspi::QspiExt as XspiExt;
Expand Down Expand Up @@ -248,7 +248,7 @@ mod common {
Error,
}

/// Indicates a specific QUADSPI bank to use
/// Indicates a specific QUADSPI bank to use.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
#[cfg(any(feature = "rm0433", feature = "rm0399"))]
Expand All @@ -260,6 +260,45 @@ mod common {
// Banks are not supported by the Octospi peripheral (there's two Octospi
// peripherals instead)

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
#[cfg(any(feature = "rm0433", feature = "rm0399"))]
pub enum BankError {
DualModeNotSupported,
}

#[cfg(any(feature = "rm0433", feature = "rm0399"))]
impl TryFrom<Bank> for BankSelect {
type Error = BankError;

fn try_from(value: Bank) -> Result<Self, Self::Error> {
match value {
Bank::One => Ok(BankSelect::One),
Bank::Two => Ok(BankSelect::Two),
Bank::Dual => Err(BankError::DualModeNotSupported),
}
}
}

/// Indicates one of the two existing QUADSPI bank.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
#[cfg(any(feature = "rm0433", feature = "rm0399"))]
pub enum BankSelect {
One,
Two,
}

#[cfg(any(feature = "rm0433", feature = "rm0399"))]
impl From<BankSelect> for Bank {
fn from(val: BankSelect) -> Self {
match val {
BankSelect::One => Bank::One,
BankSelect::Two => Bank::Two,
}
}
}

/// A structure for specifying the XSPI configuration.
///
/// This structure uses builder semantics to generate the configuration.
Expand Down
104 changes: 103 additions & 1 deletion src/xspi/qspi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
stm32,
};

use super::{Bank, Config, Qspi, SamplingEdge};
use super::{common::BankSelect, Bank, Config, Qspi, QspiError, SamplingEdge};

/// Used to indicate that an IO pin is not used by the QSPI interface.
pub struct NoIo {}
Expand All @@ -28,6 +28,9 @@ pub trait PinIo1Bank2 {}
pub trait PinIo2Bank2 {}
pub trait PinIo3Bank2 {}

/// Indicates a set of pins can be used for the QSPI interface on bank 1 and 2.
pub trait PinsBank1And2 {}

pub trait PinSck {}

impl<SCK, IO0, IO1, IO2, IO3> PinsBank1 for (SCK, IO0, IO1, IO2, IO3)
Expand All @@ -50,6 +53,41 @@ where
{
}

impl<
SCK,
BK1_IO0,
BK1_IO1,
BK1_IO2,
BK1_IO3,
BK2_IO0,
BK2_IO1,
BK2_IO2,
BK2_IO3,
> PinsBank1And2
for (
SCK,
BK1_IO0,
BK1_IO1,
BK1_IO2,
BK1_IO3,
BK2_IO0,
BK2_IO1,
BK2_IO2,
BK2_IO3,
)
where
SCK: PinSck,
BK1_IO0: PinIo0Bank1,
BK1_IO1: PinIo1Bank1,
BK1_IO2: PinIo2Bank1,
BK1_IO3: PinIo3Bank1,
BK2_IO0: PinIo0Bank2,
BK2_IO1: PinIo1Bank2,
BK2_IO2: PinIo2Bank2,
BK2_IO3: PinIo3Bank2,
{
}

macro_rules! pins {
(Bank1: [IO0: [$($IO0:ty),*] IO1: [$($IO1:ty),*] IO2: [$($IO2:ty),*] IO3: [$($IO3:ty),*]]) => {
$(
Expand Down Expand Up @@ -167,6 +205,18 @@ pub trait QspiExt {
CONFIG: Into<Config>,
PINS: PinsBank2;

fn bank_switch<CONFIG, PINS>(
self,
_pins: PINS,
initial_bank: BankSelect,
config: CONFIG,
clocks: &CoreClocks,
prec: rec::Qspi,
) -> Qspi<stm32::QUADSPI>
where
CONFIG: Into<Config>,
PINS: PinsBank1And2;

fn qspi_unchecked<CONFIG>(
self,
config: CONFIG,
Expand All @@ -179,6 +229,25 @@ pub trait QspiExt {
}

impl Qspi<stm32::QUADSPI> {
/// Switches between single-bank mode on Bank 1 and single-bank mode on Bank 2.
/// Note that it has no effect in dual-flash mode.
pub fn change_bank_unchecked(
&mut self,
bank: BankSelect,
) -> Result<(), QspiError> {
// Ensure that the peripheral is no longer busy before changing FSEL and DFM bits
if self.is_busy().is_err() {
return Err(QspiError::Busy);
};

self.rb.cr.modify(|_, w| w.dfm().clear_bit());
match bank {
BankSelect::One => self.rb.cr.modify(|_, w| w.fsel().clear_bit()),
BankSelect::Two => self.rb.cr.modify(|_, w| w.fsel().set_bit()),
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ElouanPetereau ElouanPetereau Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Ok(())
}

pub fn qspi_unchecked<CONFIG>(
regs: stm32::QUADSPI,
config: CONFIG,
Expand Down Expand Up @@ -275,6 +344,10 @@ impl Qspi<stm32::QUADSPI> {
}

impl QspiExt for stm32::QUADSPI {
/// Create and initialize a new QUADSPI peripheral that will use the single-bank mode on the Bank 1 of the flash memory.
///
/// A list of pins `(sck, io0, io1, io2, io3)` for this QSPI peripheral should be passed as pins.
/// Even if the pins are not used, the function will consume them to avoid accessing to them manually.
fn bank1<CONFIG, PINS>(
self,
_pins: PINS,
Expand All @@ -289,6 +362,10 @@ impl QspiExt for stm32::QUADSPI {
Qspi::qspi_unchecked(self, config, Bank::One, clocks, prec)
}

/// Create and initialize a new QUADSPI peripheral that will use the single-bank mode on the Bank 2 of the flash memory.
///
/// A list of pins `(sck, io0, io1, io2, io3)` for this QSPI peripheral should be passed as pins.
/// Even if the pins are not used, the function will consume them to avoid accessing to them manually.
fn bank2<CONFIG, PINS>(
self,
_pins: PINS,
Expand All @@ -303,6 +380,31 @@ impl QspiExt for stm32::QUADSPI {
Qspi::qspi_unchecked(self, config, Bank::Two, clocks, prec)
}

/// Create and initialize a new QUADSPI peripheral that can switch between both banks of the flash memory.
///
/// The Bank to use at initialization is given by the `initial_bank` parameter.
/// A list of pins `(sck, bank1_io0, bank1_io1, bank1_io2, bank1_io3, bank2_io0, bank2_io1, bank2_io2, bank2_io3)` for this QSPI peripheral should be passed as pins.
/// Even if the pins are not used, the function will consume them to avoid accessing to them manually.
///
/// Note that the peripheral will still be initialized in single-bank mode and the used bank have to be changed using the [`change_bank_unchecked`](../xspi/struct.Qspi.html#method.change_bank_unchecked) method.
fn bank_switch<CONFIG, PINS>(
self,
_pins: PINS,
initial_bank: BankSelect,
config: CONFIG,
clocks: &CoreClocks,
prec: rec::Qspi,
) -> Qspi<stm32::QUADSPI>
where
CONFIG: Into<Config>,
PINS: PinsBank1And2,
{
Qspi::qspi_unchecked(self, config, initial_bank.into(), clocks, prec)
}

/// Create and initialize a new QUADSPI peripheral without consuming any pins.
///
/// The given `bank` allow to chose between communication to just one of the two banks (single-bank mode) or to both at the same time (dual-flash mode).
fn qspi_unchecked<CONFIG>(
self,
config: CONFIG,
Expand Down
Loading