MB85RSxx FRAM bringup #1757
MB85RSxx FRAM bringup #1757
Conversation
drv/mb85rsxx-fram/src/lib.rs
Outdated
| // 0x03 & 0b0001_1111 = 3 | ||
| // 0x23 & 0b0001_1111 = 3 | ||
| // 2^3 = 8 | ||
| // 8 * 1024 * 8 = 8192 bytes |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
ah, thanks --- good catch!
Thanks! I try :) |
| pub fn new(spi: SpiDevice<S>) -> Result<Self, FramInitError> { | ||
| // Look at the FRAM's ID to make sure it's the device we expect it to | ||
| // be. In particular, make sure it's the size we think it is. | ||
| let id = FramId::read(&spi).map_err(FramInitError::SpiError)?; |
There was a problem hiding this comment.
This may not apply to FRAMs since they're faster than Flash, but is there every a situation where the FRAM might be "busy" here?
I'm asking because the SPI flash that use a similar protocol can be in a polled-busy state where you need to check the status register before they'll respond to anything else, and if you crash while they're in that state it's important to be able to come back up without just erroring.
There was a problem hiding this comment.
Per the FRAM datasheets, I don't believe there is such a state; at the very least, the status register only contains bits related to various forms of write protection (block-level, global, and status-register write protect). There isn't anything in the datasheet related to a busy state, which I think indicates that the FRAM is fast enough that we don't need to worry about waiting for operations to complete. It's certainly not exposed in the protocol.
drv/mb85rsxx-fram/src/lib.rs
Outdated
| // > infinitely. | ||
|
|
||
| // Start the write command. | ||
| let _cs_is_held_low_while_this_exists = |
There was a problem hiding this comment.
Huuuuuh. So, this can work, but it's an unexpected use for locking the SPI controller (unexpected to me, anyway). You're basically using it to fake scatter/gather.
I can't really recommend a better approach right now, but this suggests to me that the SPI API wants to be extended to do scatter-gather like the I2C API has.
There was a problem hiding this comment.
This comment in the SPI API implied to me that the lock operation was intended to be used this way:
Lines 217 to 222 in acd41c7
But, you're right that support for a scatter-gather type API would be nice --- in particular, it would let us avoid an IPC roundtrip in between writing the command and the data, etc.
|
Okay, @cbiffle, I think that I've addressed all your feedback --- let me know what you think! |
| /// Returns the size in bytes of the FRAM chip, based on its product ID. | ||
| pub(super) const fn size(product_id: u16) -> usize { | ||
| // The first 5 bits of the product ID give the density of the FRAM chip | ||
| // in multiples of 2KiB. |
There was a problem hiding this comment.
question: At the risk of sounding stupid: Is "multiples of" correct? I would read eg. 0x3 to become 3 * 2 KiB, not the 2 ^ 3 * 1 KiB that it actually means.
There was a problem hiding this comment.
yeah, i think i should have said..."exponents of two", or something, here? but "exponents of two KiB" also sounds wrong, it sounds like it's saying 2KiB ^ 3, which is also not what it is...
There was a problem hiding this comment.
Maybe "give the kilobyte density of [] in powers of two" or "give the density of [] in kilobyte powers of two"?
This branch adds an initial implementation of a driver for the Fujitsu
MB85xx series of ferroelectric RAM (FRAM) devices, as seen in Power
Shelf Controller rev C boards. The FRAM chip is a SPI device with a
fairly simple protocol, detailed in the Fujitsu datasheet. It also
has additional pins for toggling the write protection feature (which can
also be controlled over SPI) and a "hold" pin to maintain a read/write
transaction when CS is deasserted, but my driver doesn't currently use
those pins because we don't really need them.
I've also included a simple demo task which writes some data to a FRAM
device and checks whether it can be read back. Eventually, I'd like to
replace this with a more general-purpose "read the contents of the FRAM"
task that can be used for debugging purposes, but I've left it as is for
now in the interests of getting the driver implementation ready to
merge. I'll revisit this later.