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.

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

Split ServerImpl versus FlashDriver, add FailServer #1852

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

mkeeter
Copy link
Collaborator

@mkeeter mkeeter commented Aug 20, 2024

Staged on top of #1851

This PR splits the FMC NOR flash into FlashDriver (talking to the FPGA and flash) and ServerImpl (implementing the HostFlash API). In addition, it adds a non-panicking failure path. In this new failure path, there's a small Idol server which replies with an error to any message.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This seems good to me! I had a couple smallish notes, but nothing major.

Comment on lines +305 to +331
/// Failure function, running an Idol response loop that always returns an error
fn fail(err: drv_hf_api::HfError) {
let mut buffer = [0; hf::idl::INCOMING_SIZE];
let mut server = hf::FailServer { err };
loop {
idol_runtime::dispatch(&mut buffer, &mut server);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice approach to having a driver go into "failure mode" when hardware can't be initialized, I wonder if we might want to do something similar in e.g. gimlet_seq, here:

// Initializing the sequencer failed.
Err(_) => {
// Tell everyone that something's broken, as loudly as possible.
ringbuf_entry!(Trace::StartFailed(SeqError::I2cFault));
// Leave FAULT_PIN_L low (which is done at the start of init)
// All these moments will be lost in time, like tears in rain...
// Time to die.
loop {
// Sleeping with all bits in the notification mask clear means
// we should never be notified --- and if one never wakes up,
// the difference between sleeping and dying seems kind of
// irrelevant. But, `rustc` doesn't realize that this should
// never return, we'll stick it in a `loop` anyway so the main
// function can return `!`
sys_recv_notification(0);
}
}

rather than just waiting for a non-existent notification?

Obviously not something to do in this PR, but could be nice to do later.

drv/cosmo-hf/src/main.rs Show resolved Hide resolved
@mkeeter mkeeter force-pushed the mkeeter/cosmo-hf-cleanup branch 5 times, most recently from 3f54152 to d4c3ffc Compare August 21, 2024 18:33
Base automatically changed from mkeeter/cosmo-hf to master August 23, 2024 17:42
@mkeeter mkeeter enabled auto-merge (squash) August 23, 2024 17:53
@mkeeter mkeeter merged commit 2f24dd4 into master Aug 23, 2024
106 checks passed
@mkeeter mkeeter deleted the mkeeter/cosmo-hf-cleanup branch August 23, 2024 18:00
mkeeter added a commit that referenced this pull request Aug 29, 2024
Staged on top of #1852

Unlike Gimlet, Cosmo / Grapefruit only has a single 128 MiB flash chip.
This PR allows us to treat it as two virtual 64 MiB devices, preserving
the existing `HostFlash` API semantics.

---------

Co-authored-by: Nathanael Huffman <nathanael@oxidecomputer.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants