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

Add support for Bonus Data in Cosmo host flash #1991

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

mkeeter
Copy link
Collaborator

@mkeeter mkeeter commented Jan 23, 2025

In Cosmo and Grapefruit, we divide a 128 MiB flash chip into two (now three) sections:

  • 32 MiB of slot 0 (host boot data)
  • 32 MiB of slot 1 (host boot data)
  • 64 MiB of Bonus Data (previously unused)

The host boot slots use the lowest 64 KiB for "persistent data", which right now encodes which slot to select at boot time. This behavior is identical to Gimlet, and is unchanged in the PR.

This PR consists of three standalone commits, which do the following:

  • Check flash addresses in the cosmo-hf driver, to avoid writing into other sections by accident
  • Use a strong type for an absolute address (versus u32 for a section-relative address)
  • Add new Hiffy APIs to read and write the upper 64 MiB of flash
    • These APIs are added to the HostFlash interface, so they're also added to Gimlet's host flash driver, which just returns an error if you try to use them.
    • I'm open to naming suggestions here, since bonus_ is a bit awkward (but "auxiliary" is already in use)

Under the hood, the W25Q01JV uses two 64 MiB dies and exposes manual APIs to select which die is active (e.g. checking BUSY status only reads from the active die). However, all of the QSPI commands that we use take a 4-byte address and automatically switch between dies, so everything Just Works™ (feel free to double-check this, here's the docs)

@mkeeter mkeeter force-pushed the mkeeter/cosmo-hf-bonus-data branch 2 times, most recently from db505d7 to 672f899 Compare January 23, 2025 19:54
Copy link
Contributor

@nathanaelhuffman nathanaelhuffman left a comment

Choose a reason for hiding this comment

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

A general question I have is whether this "bonus data" has a stable format even when we change (forward and backward) between host boot images.

We expect to store DIMM training info in here, and doing so with only one section, disconnected from which host image we're booting results in the following questions:

  1. Is the format guaranteed to be stable between host images, or do we need a separate copy of the data for each host image?
  2. By putting it in one location for both images, the FPGA is going to have to additionally intercept these accesses and differently translate them. This is fine and possible, but can make reasoning about the system more difficult than say putting both the bonus data after each host image and always using the original offset.
    I can probably think of ways to expose more info here to aid in debugging but it just means we'll have multiple address translations depending on the accessed address from the host.

Otherwise, the rest of this is pretty straightforward and seems fine to me, and to be clear, it's also fine to do additional address translations in the fpga so long as we cah characterize what needs to be done which I think is possible.

@mkeeter mkeeter force-pushed the mkeeter/cosmo-hf-bonus-data branch from 672f899 to d8aa8b6 Compare January 24, 2025 14:28
@mkeeter
Copy link
Collaborator Author

mkeeter commented Jan 24, 2025

I chatted briefly with @rmustacc about this, and he thinks that we'll probably want a single storage location for eMCR data – it should be stable even if the host image changes, and we always want to use it to speed up boot.

Invalidation is still a little fuzzy. Slide 40 here (internal link, NDA) suggests that "full memory training will be performed" under a variety of circumstances (including DIMMs being moved around), which makes me think the ABL is doing the right thing here (and we don't have to invalidate manually). However, we'll definitely want to test it out (moving DIMMs around, corrupting the eMCR data in flash, etc).

I agree that adding more address mappings to FPGA memory is kinda ugly, but it also seems like the most straight-forward way to handle things.

Addresses below 0x2000000 (32 MiB) will always add SP5_FLASH_OFFSET, so that the SP can select between host boot images (we do this already). The Bonus Data could either be mapped to 0x2000000-0x6000000 (requires the FPGA to add a 32 MiB offset), or 0x4000000-0x8000000 (no offset needed, this is its physical address).

We can then specify this address in the host image BIOS directory (slide 38, is this the same as APCB_NV_COPY?). Hopefully, the ABL is smart enough to ignore the data if it's not yet populated, so we won't need to change anything in the host image after writing the data.

@rmustacc
Copy link
Contributor

I chatted briefly with @rmustacc about this, and he thinks that we'll probably want a single storage location for eMCR data – it should be stable even if the host image changes, and we always want to use it to speed up boot.

This is something I would like to still spend some time with before I commit, but I think it's directionally the way we want to go. I will build up some confidence in this.

@mkeeter mkeeter force-pushed the mkeeter/cosmo-hf-bonus-data branch 2 times, most recently from 60fbdcc to 11bfc51 Compare January 27, 2025 15:52
Copy link
Collaborator

@labbott labbott left a comment

Choose a reason for hiding this comment

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

It feels like there are a lot of unwrap_lite creeping in. I don't have a great answer about what me a better approach since e.g. FlashAddr::new_unchecked also has its problems.

@mkeeter mkeeter force-pushed the mkeeter/cosmo-hf-bonus-data branch from 11bfc51 to e5dd1cb Compare January 30, 2025 18:29
@mkeeter mkeeter enabled auto-merge (squash) January 30, 2025 18:29
@mkeeter mkeeter merged commit c98911c into master Jan 30, 2025
125 checks passed
@mkeeter mkeeter deleted the mkeeter/cosmo-hf-bonus-data branch January 30, 2025 18:36
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.

4 participants