MVP implementation of `HostFlash` for Cosmo #1851

mkeeter · 2024-08-20T13:57:07Z

This PR implements the HostFlash API for the Cosmo / Grapefruit flash IC, mediated by an FPGA.

gimlet-hf is renamed to hf in a few places, because we're now using the same API for both Gimlet and Cosmo systems. There are no changes to the API itself.

drv-fmc-nor-flash is renamed to drv-cosmo-hf for symmetry with Gimlet. It is split into main.rs (implementing the actual chip wrangling) and hf.rs (implementing the HostFlash Idol API).

Many functions are left as stubs, to be implemented in later PRs. However, this PR is sufficient to program and verify the flash chip with humility qspi!

out.raw

aapoalas

Peanut gallery checking in.

drv/cosmo-hf/src/hf.rs

cbiffle · 2024-08-20T19:54:24Z

drv/cosmo-hf/src/hf.rs

+            return Err(HfError::HashError.into());
+        }
+        let begin = addr as usize;
+        // TODO: Begin may be an address beyond physical end of


I think the checked add in this case is only checking for integer wrap -- the flash part is likely to be much smaller than 2**32 so the flash addressing will wrap before the integer will.

Ah, of course! Thank you <3

aapoalas · 2024-08-20T14:38:31Z

drv/cosmo-hf/src/hf.rs

+        let begin = addr as usize;
+        // TODO: Begin may be an address beyond physical end of
+        // flash part and may wrap around.
+        let end = match begin.checked_add(len as usize) {


suggestion: Could maybe use begin.checked_add(len as usize).and_then(|end| u32::try_from(end)) to get rid of the inner u32::MAX check.

I suppose this is always running on 32 bit hardware though?

For better or worse, a lot of existing Hubris code assumes a 32-bit platform, yeah.

drv/cosmo-hf/src/hf.rs

aapoalas · 2024-08-20T15:12:36Z

drv/cosmo-hf/src/hf.rs

+            }
+        };
+        // If we knew the flash part size, we'd check against those limits.
+        let mut buf = [0u8; PAGE_SIZE_BYTES];


question: Is the idea that being is aligned at the start of a page and this can then read the flash part page-by-page (not counting the last, possibly not full page)? Is start guaranteed to be page-aligned?

Not sure if it would really make much of a difference, but you could split the range into full pages (with or without a not-full prefix, depending on if start is aligned or not) plus a not-necessarily-full suffix. That would mean you don't need to calculate size for the full pages.

I think the comment on line 159 is slightly misplaced. What this code appears to be doing is just ensuring that reads are under 256 bytes in size by reusing a buffer; it doesn't appear to be sensitive to alignment, afaict.

Yup, that comment isn't quite relevant here (we're limited by FPGA buffers, not the flash chip itself). Deleted!

drv/cosmo-hf/src/main.rs

nathanaelhuffman · 2024-08-20T21:13:55Z

drv/cosmo-hf/src/main.rs

+    }
+
+    fn clear_fifos(&self) {
+        self.write_reg(reg::SPICR, 0x8080);


There is no wait required here. The FPGA FIFO clears take single-digit clock cycles to complete, as the reset may have to cross clock domains to clear read/write pointers, but the SP is hanging off the FMC interface and can't even finish executing another read/write transaction to the FPGA before the FIFO clear has completed so this can be treated as if it happens instantly.

aapoalas · 2024-08-20T15:31:08Z

drv/cosmo-hf/src/main.rs

+}
+
+impl ServerImpl {
+    fn flash_read_id(&self) -> [u8; 20] {


praise: For an outsider like me, it's really cool seeing this lowest-of-the-low level register work happening here.

suggestion: A mountain of comments wouldn't maybe go amiss here and would be a terrific pleasure to read :)

drv/cosmo-hf/src/hf.rs

cbiffle · 2024-08-20T20:02:31Z

drv/cosmo-hf/src/hf.rs

+        &mut self,
+        _: &RecvMessage,
+    ) -> Result<HfMuxState, RequestError<HfError>> {
+        todo!()


There's a lot of TODOs here providing remote panic opportunities. Are these operations going to make sense for Cosmo? I ask because, if not, we should have two different idl files, since we can't cfg things in IDL files at the moment.

From talking with Nathanael, we do think that the existing API makes sense for Cosmo!

There are PRs coming down the line that implement these functions, e.g. #1853, but I didn't want to do too much in the MVP PR.

drv/cosmo-hf/src/main.rs

cbiffle · 2024-08-20T20:07:07Z

drv/cosmo-hf/src/main.rs

+                let k = i * 4 + j;
+                if k < len {
+                    let Some(d) = data.read() else {
+                        return Err(());


Should this use an error type? I'm surprised Clippy isn't mad about this tbh.

Mildly off-topic, but I've had good(?) use of Result<T, ()> in TryFrom<ParentEnum> for SubEnum places. In that case the error case is just "it wasn't in the subgroup you checked for". So I'm not that surprised clippy doesn't complain about this: There are some minor use cases where it's not an entirely bad choice.

cbiffle

For me the highest priority things to look at here are:

Switch the generic flash read functions to use dyn so we don't generate multiple copies of them in flash. (The traits appear object-safe.)
The use of Result<(), ()> seems suspicious, consider whether the error type could be something more useful. (I think this might have been inherited from BufWriter, but I think it's arguably wrong there too; no reason to propagate the wrongness when your IPC error types are right there.)
Comment that unwrap please, it's subtle
Make sure those todo!s will get todone, otherwise the operations shouldn't be in the IPC protocol (and we need to fork it).
The flash_foo functions toward the end could use doc comments explaining any assumptions and failure cases.

drv/cosmo-hf/src/main.rs

hawkw · 2024-08-20T22:34:50Z

drv/cosmo-hf/src/main.rs

+    #[count(skip)]
+    None,
+
+    FpgaBusy(u32),


What does the u32 mean here? It might be nice if this was a struct-like variant with a named field, so that someone inspecting the ring buffer in humility immediately knows what the integer refers to?

Renamed to spi_sr

hawkw · 2024-08-20T22:36:19Z

drv/cosmo-hf/src/main.rs

+    // Wait for the FMC to be configured
+    userlib::hl::sleep_for(1000);


how do we know that it will take less than 1000 ticks to do this? What happens if we sleep and the FMC still hasn't been configured?

What happens if we sleep and the FMC still hasn't been configured?

Panic! at the ID check

(I added a basic ping function to the sequencer API, so cosmo-hf now waits for that to return before starting up)

hawkw · 2024-08-20T22:37:36Z

drv/cosmo-hf/src/main.rs

+    fn wait_fpga_rx(&self) {
+        for i in 0.. {
+            let status = self.read_reg(reg::SPISR);
+            // Wait for the


Wait for the...?

hawkw · 2024-08-20T22:40:48Z

drv/cosmo-hf/src/main.rs

+            for i in 0..len.div_ceil(4) {
+                self.wait_fpga_rx();
+                let v = self.read_reg(reg::RX_FIFO);
+                let v = v.to_le_bytes();
+                for (j, byte) in v.iter().enumerate() {
+                    let k = i * 4 + j;
+                    if k < len {
+                        dest.write(*byte)?;
+                    }
+                }
+            }


wondered a bit if this structure, which is repeated a few times, is easily generalizable to both the flash_read and flash_write methods, but honestly...trying to factor it out is probably more trouble than it's worth...

also, it could maybe be nice if there was a comment explaining that this kinda weird loop is because we read bytes out of the FIFO in 32-bit words, and depending on the length to read, we might overshoot the requisite length and have to bail early? i was able to figure it out fairly quickly but it might not be totally obvious to everyone...

opinion: It's not too difficult code to read or understand, but a quick comment wouldn't be out of place. Words are still generally easier to understand. Also I personally love the comment-rich coding style I often see in Oxide's code, so more comments! :)

mkeeter · 2024-08-21T13:29:31Z

For me the highest priority things to look at here are:

Switch the generic flash read functions to use dyn so we don't generate multiple copies of them in flash. (The traits appear object-safe.)

The use of Result<(), ()> seems suspicious, consider whether the error type could be something more useful. (I think this might have been inherited from BufWriter, but I think it's arguably wrong there too; no reason to propagate the wrongness when your IPC error types are right there.)

Comment that unwrap please, it's subtle

Make sure those todo!s will get todone, otherwise the operations shouldn't be in the IPC protocol (and we need to fork it).

The flash_foo functions toward the end could use doc comments explaining any assumptions and failure cases.

This is all done except 2, because I opened oxidecomputer/idolatry#55 to fix it upstream.

hawkw · 2024-08-21T17:43:36Z

drv/cosmo-hf/src/hf.rs

+        if addr < SECTOR_SIZE_BYTES
+            && !matches!(protect, HfProtectMode::AllowModificationsToSector0)
+        {
+            return Err(HfError::Sector0IsReserved.into());
+        }


nit, take it or leave it: we could maybe have a function like:

fn check_addr_writable(addr: u32, protect: HfProtectMode) -> Result<(), HfError> { if addr < SECTOR_SIZE_BYTES && !matches!(protect, HfProtectMode::AllowModificationsToSector0) { return Err(HfError::Sector0IsReserved); } Ok(()) }

since we seem to repeat this in a few places. potentially, factoring it out it would mean that a future change adding other checks against HfProtectModes wouldn't have to add the requisite checks in every function that currently checks if sector 0 is writable?

not a big deal either way though.

I like it, done in 0f4cd3c

drv/cosmo-hf/src/hf.rs

hawkw · 2024-08-21T17:48:22Z

drv/cosmo-hf/src/main.rs

+fn main() -> ! {
+    // Wait for the FPGA to be configured
+    let seq = drv_grapefruit_seq_api::Sequencer::from(SEQ.get_task_id());
+    seq.ping(); // waits until the sequencer has completed configuration


Cute, I like this approach. :)

hawkw

Looks good to me, no blockers! I agree with Cliff that it would be good to prioritize checking off the todo!s relatively soon, but everything looks good to me!

hawkw · 2024-08-23T16:39:20Z

drv/cosmo-hf/src/main.rs

+        self.write_reg(reg::DATA_BYTES, 1);
+        self.write_reg(reg::ADDR, 0);
+        self.write_reg(reg::DUMMY_CYCLES, 0);
+        self.write_reg(reg::INSTR, instr::READ_STATUS_1);


there is a part of me that wonders if it's worth taking all this and putting it in a function that takes the number of bytes and the instruction value, but, honestly, I'm not sure if it would be worth doing, because you'd still have to provide the number of bytes, address, dummy cycles, and instructions to that function so i don't really think it would be abstracting over anything meaningful...and this feels more explicit, i guess?

hawkw · 2024-08-23T16:43:17Z

drv/cosmo-hf/src/main.rs

+            for i in 0..len.div_ceil(4) {
+                self.wait_fpga_rx();
+                let v = self.read_reg(reg::RX_FIFO);
+                let v = v.to_le_bytes();
+                for (j, byte) in v.iter().enumerate() {
+                    let k = i * 4 + j;
+                    if k < len {
+                        dest.write(*byte)?;
+                    }
+                }
+            }


also, it could maybe be nice if there was a comment explaining that this kinda weird loop is because we read bytes out of the FIFO in 32-bit words, and depending on the length to read, we might overshoot the requisite length and have to bail early? i was able to figure it out fairly quickly but it might not be totally obvious to everyone...

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

mkeeter requested review from cbiffle, hawkw, labbott and nathanaelhuffman August 20, 2024 13:57

This comment was marked as resolved.

Sign in to view

nathanaelhuffman reviewed Aug 20, 2024

View reviewed changes

out.raw Outdated Show resolved Hide resolved

mkeeter mentioned this pull request Aug 20, 2024

Split ServerImpl versus FlashDriver, add FailServer #1852

Merged

aapoalas reviewed Aug 20, 2024

View reviewed changes

cbiffle reviewed Aug 20, 2024

View reviewed changes

drv/cosmo-hf/src/hf.rs Outdated Show resolved Hide resolved

cbiffle reviewed Aug 20, 2024

View reviewed changes

drv/cosmo-hf/src/main.rs Outdated Show resolved Hide resolved

cbiffle reviewed Aug 20, 2024

View reviewed changes

mkeeter force-pushed the mkeeter/cosmo-hf branch from cd00656 to d4adadd Compare August 20, 2024 21:48

hawkw reviewed Aug 20, 2024

View reviewed changes

hawkw reviewed Aug 21, 2024

View reviewed changes

mkeeter force-pushed the mkeeter/cosmo-hf branch from 5892fa3 to e3dcb40 Compare August 21, 2024 18:49

hawkw approved these changes Aug 23, 2024

View reviewed changes

mkeeter and others added 11 commits August 23, 2024 13:34

Implementing HostFlash API for cosmo

83e9828

Count dummies on redge of sclk, so we don't accidentally skip one

b5a9a9b

Fix sector erase to use 4B addresses

d9e977d

Add license

b6e80c5

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Remove debug logging

d993178

Remove dead code

5a10404

Remove accidental file

14e7b97

Changes per review

b56e90a

thanks, clippy

44b2fe4

Wait for sequencer task before starting cosmo-hf

faeb285

Helper function to check sector 0 protection

28006c7

mkeeter added 2 commits August 23, 2024 13:34

Log hash driver init failure

0a48496

clipppyyyyy

9967a30

mkeeter force-pushed the mkeeter/cosmo-hf branch from e3dcb40 to 9967a30 Compare August 23, 2024 17:35

mkeeter enabled auto-merge (squash) August 23, 2024 17:36

mkeeter merged commit 66721b1 into master Aug 23, 2024
106 checks passed

mkeeter deleted the mkeeter/cosmo-hf branch August 23, 2024 17:42

mkeeter mentioned this pull request Oct 3, 2024

Hubris: Host OS QSPI Driver via FPGA #1790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVP implementation of `HostFlash` for Cosmo #1851

MVP implementation of `HostFlash` for Cosmo #1851

mkeeter commented Aug 20, 2024

This comment was marked as resolved.

aapoalas left a comment

This comment was marked as resolved.

cbiffle Aug 20, 2024

aapoalas Aug 20, 2024

aapoalas Aug 20, 2024 •

edited

Loading

cbiffle Aug 20, 2024

aapoalas Aug 20, 2024

cbiffle Aug 20, 2024

mkeeter Aug 20, 2024

This comment was marked as resolved.

nathanaelhuffman Aug 20, 2024

aapoalas Aug 20, 2024

cbiffle Aug 20, 2024

mkeeter Aug 20, 2024

cbiffle Aug 20, 2024

aapoalas Aug 20, 2024

cbiffle left a comment

hawkw Aug 20, 2024

mkeeter Aug 21, 2024

hawkw Aug 20, 2024

mkeeter Aug 21, 2024

hawkw Aug 20, 2024

mkeeter Aug 21, 2024

hawkw Aug 20, 2024

hawkw Aug 23, 2024

aapoalas Aug 23, 2024

mkeeter commented Aug 21, 2024

hawkw Aug 21, 2024

mkeeter Aug 21, 2024

hawkw Aug 21, 2024

hawkw left a comment

hawkw Aug 23, 2024

hawkw Aug 23, 2024

		// Wait for the FMC to be configured
		userlib::hl::sleep_for(1000);

MVP implementation of HostFlash for Cosmo #1851

MVP implementation of HostFlash for Cosmo #1851

Conversation

mkeeter commented Aug 20, 2024

This comment was marked as resolved.

aapoalas left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapoalas Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiffle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVP implementation of `HostFlash` for Cosmo #1851

MVP implementation of `HostFlash` for Cosmo #1851

aapoalas Aug 20, 2024 •

edited

Loading