Switch SPI master HIL to leasable buffers instead of raw slices #4173
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Migrates SPI HIL to leasable buffers (SubSliceMut) rather than raw slices, in accordance with the draft SPI TRD
696dd29 to
cd411b4
Compare
bradjc
reviewed
Sep 17, 2024
| status: Result<(), ErrorCode>, | ||
| write_buffer: SubSliceMut<'static, u8>, | ||
| read_buffer: Option<SubSliceMut<'static, u8>>, | ||
| status: Result<usize, ErrorCode>, |
There was a problem hiding this comment.
Can you update the comment?
bradjc
approved these changes
Sep 19, 2024
|
@tock/core-wg note last call on this |
2 tasks
|
This breaks SPI on the Apollo3 |
alistair23
reviewed
Oct 2, 2024
Comment on lines
-138
to
+143
| self.spi_master | ||
| .read_write_bytes(self.kernel_write.take().unwrap(), None, write_len) | ||
| let kwbuf = self | ||
| .kernel_write | ||
| .take() | ||
| .unwrap_or((&mut [] as &'static mut [u8]).into()); | ||
| self.spi_master.read_write_bytes(kwbuf, None) |
There was a problem hiding this comment.
What happens to write_len here? It seems like it's being lost
alistair23
reviewed
Oct 2, 2024
Comment on lines
+172
to
+177
| let kwbuf = self | ||
| .kernel_write | ||
| .take() | ||
| .unwrap_or((&mut [] as &'static mut [u8]).into()); | ||
| self.spi_master | ||
| .read_write_bytes(kwbuf, self.kernel_read.take()) |
There was a problem hiding this comment.
Same with read_len here.
Userspace might use a single large buffer, but want to do smaller SPI operations at times. It seems like that might be broken now
alistair23
reviewed
Oct 2, 2024
Comment on lines
-174
to
+184
| self.spi_master.read_write_bytes( | ||
| self.kernel_write.take().unwrap(), | ||
| self.kernel_read.take(), | ||
| write_len, | ||
| ) | ||
| let kwbuf = self | ||
| .kernel_write | ||
| .take() | ||
| .unwrap_or((&mut [] as &'static mut [u8]).into()); | ||
| self.spi_master | ||
| .read_write_bytes(kwbuf, self.kernel_read.take()) |
There was a problem hiding this comment.
Same with write_len here
|
@alevy ping! |
alistair23
added a commit
to alistair23/tock
that referenced
this pull request
Nov 13, 2024
tock#4173 broke userspace SPI operations as the length of the operation that the application requested was ignored see [1] and [2]. This commit fixes SPI operations by ensuring that we only write the data requested by the application, not just the entire buffer. 1: https://github.com/tock/tock/pull/4173/files#r1783773610 2: https://github.com/tock/tock/pull/4173/files#r1783774331 Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
2 tasks
alistair23
added a commit
to alistair23/tock
that referenced
this pull request
Nov 13, 2024
tock#4173 drops the length specified by the application for read only operations. This commit fixes the read buffer length before calling the SPI operation. Signed-off-by: Alistair Francis <alistair@alistair23.me>
This was referenced Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Overview
This pull request migrates the SPI master HIL to use leasable buffers (SubSliceMut) rather than raw
slices, in accordance with the draft SPI TRD. As a result, this touches every module that either implements or uses the SPI master HIL, as well as a some dependencies in some cases (e.g. DMA modules).
While it is mostly a mechanical transformation aided by some additional helper methods in leasable buffers, the length parameter, is notably, redundant for both the
read_write_bytesdowncall and client callback, which requires adapting some of the length computations in various implementations. In cases where the implementation is shared with other HIL implementations that have not migrated toSubSliceMutthere is some ultimately unnecessary complexity.Testing Strategy
This touches many chips and thus almost every board as well as a few capsules for peripherals I don't have currently access to. The shared code is fairly straight forward, but the devil is mostly going to be in specific chip implementations.
I've tested on an NRF52-based platform with a SPI peripheral, but that's it for now.
TODO or Help Wanted
SubSliceMuts in capsules... kind of everywhere?write_bufferalso be anOption?Documentation Updated
/docs, or no updates are required.Formatting
make prepush.