Skip to content

Commit 406d9b1

Browse files
committed
cosmo-hf: add limited busy-polls in all wait paths
Presently, the `cosmo-hf` driver has three functions that wait for some status to change before continuing: `wait_fpga_busy`, `wait_fpga_rx`, and `wait_flash_busy`. All of these functions check the desired condition in a loop and sleep for 1ms when it is not satisfied. Currently, `wait_fpga_busy` will first perform a limited number of busy-polls before starting to sleep between polls, but the other functions do not. While investigating HF write performance on Grapefruit, @cbiffle and I found that commenting out the sleeps in these polling functions improved HF write performance significantly: writing 2MiB of random data using `humility qspi -D` went from taking about a minute to about 36 seconds --- that's a 40% performance improvement. This branch is like a slightly more principled version of commenting out the `sleep_for` calls. I've generalized the pervious `wait_fpga_busy` of doing 32 busy-polls before starting to sleep to something that can be used by all three wait-for-condition type functions. I did change the code for implementing this a bit from the previous thing, which implemented the counter using a `for i in 0..` loop that checked `i > 32` This was a bit more concise and kind of a cute trick, but had a somewhat surprising behavior: the loop wasn't actually infinite, and would terminate after `u32::MAX` polls. Of course, polling for 4,294,967,295ms means we would have waited about 50 days for a write to complete before terminating the loop unexpectedly, and if that happened, something would have to be REALLY broken. But, it's still a pretty wrong behavior, so the implementation I've done --- using a `while` loop with an explicitly incremented counter --- will never terminate unless the condition has actually changed, which seems a little less anxiety-inducing.
1 parent 1fd855b commit 406d9b1

File tree

1 file changed

+40
-21
lines changed

1 file changed

+40
-21
lines changed

drv/cosmo-hf/src/main.rs

+40-21
Original file line numberDiff line numberDiff line change
@@ -145,27 +145,12 @@ impl FlashDriver {
145145

146146
/// Wait until the FPGA is idle
147147
fn wait_fpga_busy(&self) {
148-
loop {
149-
if !self.drv.spisr.busy() {
150-
break;
151-
}
152-
ringbuf_entry!(Trace::FpgaBusy);
153-
sleep_for(1);
154-
}
148+
self.poll_wait(|this| !this.drv.spisr.busy(), Trace::FpgaBusy)
155149
}
156150

157151
/// Wait until a word is available in the FPGA's RX buffer
158152
fn wait_fpga_rx(&self) {
159-
for i in 0.. {
160-
if !self.drv.spisr.rx_empty() {
161-
break;
162-
}
163-
ringbuf_entry!(Trace::FpgaBusy);
164-
// Initial busy-loop for faster response
165-
if i >= 32 {
166-
sleep_for(1);
167-
}
168-
}
153+
self.poll_wait(|this| !this.drv.spisr.rx_empty(), Trace::FpgaBusy)
169154
}
170155

171156
/// Clears the FPGA's internal FIFOs
@@ -177,13 +162,47 @@ impl FlashDriver {
177162
});
178163
}
179164

165+
/// Wait for a condition represented by the provided `poll` function.
166+
///
167+
/// The driver will wait until the `poll` function returns `true`. Each time
168+
/// `poll` returns `false`, the provided `trace` will be recorded in the
169+
/// ring buffer.
170+
fn poll_wait(&self, poll: fn(&Self) -> bool, trace: Trace) {
171+
// When polling the FPGA or flash chips, this number of polls are
172+
// attempted *without* sleeping between polls. If the FPGA/flash's
173+
// status has not changed after this number of polls, the driver will
174+
// begin to sleep for a short period between subsequent polls.
175+
//
176+
// This is intended to improve copy performance for operations where the
177+
// desired status transition occurs in less than 1ms, avoiding a 1-2ms
178+
// sleep and round-trip through the scheduler. status transitions
179+
// quickly.
180+
const MAX_BUSY_POLLS: u32 = 32;
181+
182+
let mut busy_polls = 0;
183+
while !poll(self) {
184+
ringbuf_entry!(trace);
185+
186+
if busy_polls > MAX_BUSY_POLLS {
187+
// If we've exhausted all of our busy polls, sleep for a bit
188+
// before polling again.
189+
sleep_for(1);
190+
} else {
191+
// Only increment the counter while we are busy-polling.
192+
// Otherwise, if we incremented it unconditionally, we might
193+
// overflow and start busy-polling again. Of course, we won't do
194+
// that unless we are stuck waiting for 4,294,967,295ms, which
195+
// is a little under 50 days, so things would probably have gone
196+
// very wrong if that happened. But, still...
197+
busy_polls += 1;
198+
}
199+
}
200+
}
201+
180202
/// Wait until the SPI flash is idle
181203
fn wait_flash_busy(&self, t: Trace) {
182204
// Wait for the busy flag to be unset
183-
while (self.read_flash_status() & 1) != 0 {
184-
ringbuf_entry!(t);
185-
sleep_for(1);
186-
}
205+
self.poll_wait(|this| this.read_flash_status() & 1 == 0, t);
187206
}
188207

189208
/// Reads the STATUS1 register from flash

0 commit comments

Comments
 (0)