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

MB85RSxx FRAM bringup #1757

Merged
merged 23 commits into from
Jun 11, 2024
Merged

MB85RSxx FRAM bringup #1757

merged 23 commits into from
Jun 11, 2024

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 19, 2024

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.

@hawkw hawkw requested a review from cbiffle April 19, 2024 22:10
Copy link
Contributor

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

I'm not sure if outside reviews are welcome here: My apologies if I'm just being a nuisance.

I just wanted to say that this was really nice and easy to read, and all the code looked good to me.

// 0x03 & 0b0001_1111 = 3
// 0x23 & 0b0001_1111 = 3
// 2^3 = 8
// 8 * 1024 * 8 = 8192 bytes

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, thanks --- good catch!

@hawkw
Copy link
Member Author

hawkw commented Apr 22, 2024

I just wanted to say that this was really nice and easy to read, and all the code looked good to me.

Thanks! I try :)

drv/mb85rsxx-fram/src/lib.rs Outdated Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Outdated Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Outdated Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Outdated Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Show resolved Hide resolved
drv/mb85rsxx-fram/src/lib.rs Outdated Show resolved Hide resolved
// > infinitely.

// Start the write command.
let _cs_is_held_low_while_this_exists =
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment in the SPI API implied to me that the lock operation was intended to be used this way:

/// `assert_cs` can be used to force CS into the asserted (low) state, or
/// keep it deasserted. If you choose to assert it, then SPI transactions
/// via `read`/`write`/`exchange` will leave it asserted rather than
/// toggling it. You can call `lock` while the SPI controller is locked (by
/// you) to alter CS state, either to toggle it on its own, or to enable
/// per-transaction CS control again.

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.

@hawkw hawkw requested a review from cbiffle April 22, 2024 21:47
@hawkw
Copy link
Member Author

hawkw commented Apr 22, 2024

Okay, @cbiffle, I think that I've addressed all your feedback --- let me know what you think!

@hawkw hawkw enabled auto-merge (squash) June 11, 2024 16:15
@hawkw hawkw merged commit ceb2908 into master Jun 11, 2024
104 checks passed
@hawkw hawkw deleted the eliza/mb86rs branch June 11, 2024 16:20
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.

None yet

4 participants