Skip to content

chore: Minor QSPI code deduplication #2095

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+60 −69
Open

chore: Minor QSPI code deduplication #2095

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8be3fba
chore(stm32h7-qspi): Deduplicate QSPI interrupt handling
aapoalas Jun 13, 2025
54bd5be
refactor: disable all interrupts outside of inner impls
aapoalas Jul 25, 2025
77fab69
perf: remove unnecessary panic in Qspi::write_impl_inner
aapoalas Jul 27, 2025
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
129 changes: 60 additions & 69 deletions drv/stm32h7-qspi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ impl Qspi {
}

/// Helper for error paths.
///
/// Disables FIFO Threshold, Transfer Complete, Transfer Error, and Timeout
/// interrupts.
fn disable_all_interrupts(&self) {
self.reg.cr.modify(|_, w| {
w.ftie()
Expand All @@ -180,16 +183,60 @@ impl Qspi {
});
}

/// Returns the number of valid bytes being held in the FIFO queue if a
/// QSPI timeout or transfer error hasn't occurred.
#[inline(always)]
fn get_fifo_level(&self) -> Result<usize, QspiError> {
let sr = self.reg.sr.read();

// Check timeout bit.
if sr.tof().bit_is_set() {
// Clear timeout bit and return.
self.reg.fcr.modify(|_, w| w.ctof().set_bit());
return Err(QspiError::Timeout);
}
// Check transfer error bit.
if sr.tef().bit_is_set() {
// Clear transfer error bit and return.
self.reg.fcr.modify(|_, w| w.ctef().set_bit());
return Err(QspiError::TransferError);
}
// Re-enable transfer error and timeout interrupts to make sure we
// catch any future errors.
self.reg
.cr
.modify(|_, w| w.teie().set_bit().toie().set_bit());
Ok(usize::from(sr.flevel().bits()))
}

/// Wait for the Transfer Complete flag to get set.
///
/// Note: this function returning does not guarantee that the BUSY flag is
/// clear.
#[inline(always)]
fn wait_for_transfer_complete(&self) {
// Disable FIFO threshold interrupt, and enable transfer complete
// interrupts.
self.reg
.cr
.modify(|_, w| w.ftie().clear_bit().tcie().set_bit());
while self.transfer_not_complete() {
// Unmask our interrupt.
sys_irq_control(self.interrupt, true);
// And wait for it to arrive.
sys_recv_notification(self.interrupt);
}
}

fn write_impl(
&self,
command: Command,
addr: Option<u32>,
data: &[u8],
) -> Result<(), QspiError> {
let result = self.write_impl_inner(command, addr, data);
if result.is_err() {
self.disable_all_interrupts();
}
// Clean up by disabling our interrupt sources.
self.disable_all_interrupts();
result
}

Expand Down Expand Up @@ -238,25 +285,11 @@ impl Qspi {
// off the front.
let mut data = data;
while !data.is_empty() {
let sr = self.reg.sr.read();

if sr.tof().bit_is_set() {
self.reg.fcr.modify(|_, w| w.ctof().set_bit());
return Err(QspiError::Timeout);
}
if sr.tef().bit_is_set() {
self.reg.fcr.modify(|_, w| w.ctef().set_bit());
return Err(QspiError::TransferError);
}

// Make sure our errors are enabled
self.reg
.cr
.modify(|_, w| w.teie().set_bit().toie().set_bit());
// Check for any errors
let fl = self.get_fifo_level()?;

// How much space is in the FIFO?
let fl = usize::from(sr.flevel().bits());
let ffree = FIFO_SIZE - fl;
let ffree = FIFO_SIZE.saturating_sub(fl);
if ffree >= FIFO_THRESH.min(data.len()) {
// Calculate the write size. Note that this may be bigger than
// the threshold used above above. We'll opportunistically
Expand Down Expand Up @@ -293,18 +326,8 @@ impl Qspi {
}

// We're now interested in transfer complete, not FIFO ready.
self.reg
.cr
.modify(|_, w| w.ftie().clear_bit().tcie().set_bit());
while self.transfer_not_complete() {
// Unmask our interrupt.
sys_irq_control(self.interrupt, true);
// And wait for it to arrive.
sys_recv_notification(self.interrupt);
}
self.reg.cr.modify(|_, w| {
w.tcie().clear_bit().teie().clear_bit().toie().clear_bit()
Copy link
Contributor Author

@aapoalas aapoalas Jul 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

note: So here the code previously cleared just TCIE, TEIE, and TOIE. Now the Ok-path goes into disable_all_interrupts, clearing one more register which then causes a 4 byte increase in code size.

I would've actually thought the compiler could combine the error and success paths here but that doesn't seem to be the case; the 4 byte increase suggests that the Ok-path is independent of the Err-path, even though they both do the same thing.

});
self.wait_for_transfer_complete();

Ok(())
}

Expand All @@ -315,9 +338,8 @@ impl Qspi {
out: &mut [u8],
) -> Result<(), QspiError> {
let result = self.read_impl_inner(command, addr, out);
if result.is_err() {
self.disable_all_interrupts();
}
// Clean up by disabling our interrupt sources.
self.disable_all_interrupts();
result
}

Expand Down Expand Up @@ -375,22 +397,10 @@ impl Qspi {
// perform transfers.
let mut out = out;
while !out.is_empty() {
let sr = self.reg.sr.read();
// Get the FIFO level if no errors have occurred.
let fl = self.get_fifo_level()?;

if sr.tof().bit_is_set() {
self.reg.fcr.modify(|_, w| w.ctof().set_bit());
return Err(QspiError::Timeout);
}
if sr.tef().bit_is_set() {
self.reg.fcr.modify(|_, w| w.ctef().set_bit());
return Err(QspiError::TransferError);
}
// Make sure our errors are enabled
self.reg
.cr
.modify(|_, w| w.teie().set_bit().toie().set_bit());
// Is there enough to read that we want to bother with it?
let fl = usize::from(sr.flevel().bits());
if fl < FIFO_THRESH.min(out.len()) {
// Nope! Let's wait for more bytes.

Expand Down Expand Up @@ -438,27 +448,8 @@ impl Qspi {
// necessarily imply the BUSY flag is clear, but since commands are
// issued into a FIFO, we can issue the next command even while BUSY is
// set, it appears.
self.reg
.cr
.modify(|_, w| w.ftie().clear_bit().tcie().set_bit());
while self.transfer_not_complete() {
// Unmask our interrupt.
sys_irq_control(self.interrupt, true);
// And wait for it to arrive.
sys_recv_notification(self.interrupt);
}
self.wait_for_transfer_complete();

// Clean up by disabling our interrupt sources.
self.reg.cr.modify(|_, w| {
w.ftie()
.clear_bit()
.tcie()
.clear_bit()
.teie()
.clear_bit()
.toie()
.clear_bit()
});
Ok(())
}

Expand Down