[RFC] Spi controller#134
Conversation
jerrysxie
left a comment
jerrysxie
left a comment
There was a problem hiding this comment.
The interface looks good. Instead of focusing on the interface for now, can we focus on getting blocking spi hal up and running. While implementing, we will find quirks in the design and hardware that might feed backs into the design as well.
|
Please, make sure to fix the errors and warnings reported by the workflow checkers. |
There was a problem hiding this comment.
These should be optional, no?
There was a problem hiding this comment.
Can change them to Option<>, but not sure yet how to handle the later into_ref ...
into_ref!(ssel0);
There was a problem hiding this comment.
Looking at SpiBus (this WIP), compared to SpiDevice hal apis, it is SpiDevice that includes chip select. So, these actually need to move to a SpiDevice api implementation.
There was a problem hiding this comment.
impl Peripheral<P = impl *Pin<T>> + 'd, also chip selects should be optional
There was a problem hiding this comment.
Looking at SpiBus (this WIP), compared to SpiDevice hal apis, it is SpiDevice that includes chip select. So, these actually need to move to a SpiDevice api implementation.
There was a problem hiding this comment.
How about updating your macro such that you can use like so:
impl_miso!(FLEXCOMM0, PIO0_1, F1, PIO3_0, F5);(and similarly for other pins)
There was a problem hiding this comment.
hmm, user can decide how many chip selects to use, right? How about taking a slice of chip selects?
fn new_inner<T: Instance>(
_bus: impl Peripheral<P = T> + 'd,
configuration: Config,
sclk: impl Peripheral<P = impl SclkPin<T>> + 'd,
mosi: impl Peripheral<P = impl MosiPin<T>> + 'd,
miso: impl Peripheral<P = impl MisoPin<T>> + 'd,
ssel: &mut [impl Peripheral<P = impl SselPin<T>> + 'd],
dma_ch_rx: Option<dma::channel::ChannelAndRequest<'d>>,
dma_ch_tx: Option<dma::channel::ChannelAndRequest<'d>>,
) -> Result<Self> {
// ...
}There was a problem hiding this comment.
Looking at SpiBus (this WIP), compared to SpiDevice hal apis, it is SpiDevice that includes chip select. So, these actually need to move to a SpiDevice api implementation.
There was a problem hiding this comment.
... then this would be carried out with a loop:
for ssel in ssel.iter_mut() {
into_ref!(ssel);
ssel.as_ssel();
}There was a problem hiding this comment.
UPDATE: I spent some time reading the embedded-hal documentation for SPI. I don't think we can use the embedded SSEL pins. According to the HAL docs, chip selects must implement OutputPin which comes from the GPIO stuff.
https://docs.rs/embedded-hal/latest/embedded_hal/spi/index.html#for-hal-authors
We could, potentially, implement OutputPin for the ssel pins and have that implementation assert the correct SSEL bit in the Spi registers, but the user should be able to instantiate SpiBus and SpiDevice according to what they're trying to achieve.
There was a problem hiding this comment.
Thank you for finding that. Looks like there will be some additional architectural work to fully support the chip select options. I am going to wait on that until I have the first level of tx/rx data working.
There was a problem hiding this comment.
Yes, looking at SpiBus (this WIP), compared to SpiDevice hal apis, it is SpiDevice that includes chip select. So, chip select behavior needs to move to a SpiDevice api implementation.
DM777-ms
force-pushed
the
spi
branch
6 times, most recently
from
0eb424f to
d8d745b
Compare
DM777-ms
force-pushed
the
spi
branch
2 times, most recently
from
29c40cf to
fe10709
Compare
DM777-ms
force-pushed
the
spi
branch
3 times, most recently
from
368328b to
afa6b38
Compare
| info: Info, | ||
| _phantom: PhantomData<M>, | ||
| config: Config, | ||
| //ssel0: Option<PeripheralRef<'d, AnyPin>>, |
There was a problem hiding this comment.
todo: remove sseln from SpiBus hal apis, move to SpiDevice hal apis
| spi_instance.info.regs.fifocfg().write(|w| | ||
| w.enablerx().enabled().enabletx().enabled().emptyrx().set_bit().emptytx().set_bit()); | ||
|
|
||
| // spi is disabled, so this just clears the chip selects, and does not initiate fifo transfers |
| miso.as_miso(); | ||
| mosi.as_mosi(); | ||
| // only enable required chip selects | ||
| if ssel0.is_some() { |
There was a problem hiding this comment.
remove ssel. Implement SpiDevice hal, move ssel to SpiDevice
5 participants
DM777-ms
DM777-ms commented last week •
SPI controller wip.
Currently implementation level: Blocking "transfer()" api can tx and rx on FC5 and EVK board, and forcing chip select 1. With an external loop back, rx matches tx.
Dependent on #235 for fc14 support.
RT685 - SPI (Master) · Issue OpenDevicePartnership/embassy-imxrt#41 · OpenDevicePartnership/embassy-imxrt
REMAINING WORK TODO
SpiBus (master):
Remove chip selects from all SpiBus (SpiController) apis, move to SpiDevice HAL implementation.
Fully support clock speed request. Select source clock that will best match requested SPI CLK, and configure SPI CLK to match the requested frequency in new().
Config options:
(Related Issue #511 Add Support for SetConfig trait · Issue OpenDevicePartnership/embassy-imxrt#511 · OpenDevicePartnership/embassy-imxrt)
-msb/lsb first
-frame delay, pre delay, post delay, transfer delay
Implement apis:
blocking_read()
blocking_write()
Blocking_tranfer() - Mostly done, but needs more testing
blocking_transfer_in_place()
blocking_flush()
async_read()
async_write()
async_transfer()
async_transfer_in_place()
async_flush()
Implement SpiDevice HAL with chip selects:
blocking_read()
blocking_write()
blocking_transfer()
blocking_transfer_in_place()
blocking_flush()
async_read()
async_write()
async_transfer()
async_transfer_in_place()
async_flush()
Dma for async apis
Related work:
Implement FC14/FC15 support.
See issue #79
Add support for flexcomm 14 and 15 · Issue OpenDevicePartnership/embassy-imxrt#79 · OpenDevicePartnership/embassy-imxrt
and PR #235
[RFC] Add support for flexcomm14 and 15 by DM777-ms · Pull Request OpenDevicePartnership/embassy-imxrt#235 · OpenDevicePartnership/embassy-imxrt