-
Notifications
You must be signed in to change notification settings - Fork 8.1k
MSPI_DW DMA and Async support #94469
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
base: main
Are you sure you want to change the base?
MSPI_DW DMA and Async support #94469
Conversation
Current known issues/to do list for this PR:
To be done in follow-up PR:
|
ce8a013
to
c095bba
Compare
266a237
to
aa46e61
Compare
3b17e2e
to
3c55ba4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't managed to go through all the code yet but there is already quite a few issues to be addressed.
8761bd1
to
2827f5e
Compare
2827f5e
to
235a0c3
Compare
Thanks very much for your review @anangl, lots of helpful comments. Hopefully the latest push fixes most of them. I've also added a few replies to your comments for your input. |
0fa1414
to
1d94cb0
Compare
/* Number of jobs needed for transmit trasaction */ | ||
#define MAX_NUM_JOBS 4 | ||
/* Just support 1 trasaction for each peripheral as concurrent transactions aren't supported yet*/ | ||
#define MAX_CONCURR_TRANSACTIONS 1 |
There was a problem hiding this 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 I understand what you actually mean by concurrent transactions and how those could be supported by the driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this, I mean multiple requests of mspi_transceive() that get buffered by the driver. Not sure if this was going to be needed but currently if another request is made while a transaction is already in progress it will just reject the request and say "Async transfer already in progress".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requests would need to be somehow queued then. I think it's better to drop this for now.
preg->CONFIG.TXBURSTLENGTH = (config->tx_fifo_depth_minus_1+1)-config->dma_tx_data_level; | ||
preg->CONFIG.RXBURSTLENGTH = config->dma_rx_data_level+1; | ||
preg->DMA.CONFIG.LISTPTR = (uint32_t)transfer_list; | ||
preg->INTEN = BIT(QSPI_INTEN_DMADONE_Pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clears BIT(QSPI_INTENSET_CORE_Pos)
, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting QSPI_INTENSET_CORE_Pos here seems to brick the system, not sure why but left a comment to address in future as I'm assuming this is needed for XiP? The system will still work in FIFO mode since this DMA function will never be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting QSPI_INTENSET_CORE_Pos here seems to brick the system, not sure why but left a comment to address in future as I'm assuming this is needed for XiP?
XIP has nothing to do with interrupts. Something is apparently wrong and it would be good clarify this.
The system will still work in FIFO mode since this DMA function will never be called.
Not really. Once you perform a DMA transfer, you won't be able to perform a PIO one anymore, as the related interrupt will be disabled.
1d94cb0
to
af0dc30
Compare
af0dc30
to
ebd36d4
Compare
Support for new MSPI peripheral where there is no PSEL so pins are setup through CTRLSEL. Signed-off-by: David Jewsbury <[email protected]>
This is a new callback option that drivers can use for when a request or xfer has timed out. E.g if an Async RX request never received anything, this callback can be used so a user can clean up their application. Signed-off-by: David Jewsbury <[email protected]>
enum mspi_op_mode in mspi.h has different syntax to this binding. Aligning these will allow for cleaner code in the implmented drivers. Signed-off-by: David Jewsbury <[email protected]>
ebd36d4
to
694e6ab
Compare
Handling of asynchronous transfers uses the system workqueue, hence they are not available when multithreading is disabled. Also add missing dependency on multithreading in the MSPI_DW_HANDLE_FIFOS_IN_SYSTEM_WORKQUEUE Kconfig option. Signed-off-by: David Jewsbury <[email protected]> Signed-off-by: Andrzej Głąbek <[email protected]>
MSPI slave mode is selected through devicetree using the op-mode property. Mode selected by SSIISMST bit in the CTRLR0 register. EXMIF can only be Master (controller). Signed-off-by: David Jewsbury <[email protected]>
Initial DMA support. DMA supports implementation of SSI IP but using vendor specific DMA in the wrapper. The setup of the DMA is done in mspi_dw_vendor_specific.h. Signed-off-by: David Jewsbury <[email protected]>
The nrf-mspi peripheral is similar to EXMIF on nrf54h20 but supports DMA and slave-mode. The wrapper around the SSI IP is also different with DMA features. Signed-off-by: David Jewsbury <[email protected]>
694e6ab
to
523f677
Compare
|
preg->CONFIG.TXBURSTLENGTH = (config->tx_fifo_depth_minus_1+1)-config->dma_tx_data_level; | ||
preg->CONFIG.RXBURSTLENGTH = config->dma_rx_data_level+1; | ||
preg->DMA.CONFIG.LISTPTR = (uint32_t)transfer_list; | ||
preg->INTEN = BIT(QSPI_INTEN_DMADONE_Pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting QSPI_INTENSET_CORE_Pos here seems to brick the system, not sure why but left a comment to address in future as I'm assuming this is needed for XiP?
XIP has nothing to do with interrupts. Something is apparently wrong and it would be good clarify this.
The system will still work in FIFO mode since this DMA function will never be called.
Not really. Once you perform a DMA transfer, you won't be able to perform a PIO one anymore, as the related interrupt will be disabled.
When in DMA mode, the transmit data level field controls the level at which a DMA request | ||
is made by the transmit logic. A request to transmit is generated when the number of | ||
valid data entries in the transmit FIFO is equal to or below this field value. Lower values | ||
mean less frequent DMA triggers with larger bursts. Higher values mean fewer, larger bursts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mean less frequent DMA triggers with larger bursts. Higher values mean fewer, larger bursts | |
mean less frequent DMA triggers with larger bursts. Higher values mean fewer, smaller bursts |
description: | | ||
When in DMA mode, the receive data level field controls the level at which a DMA request | ||
is made by the receive logic. A request to receive is generated when the number of | ||
valid data entries in the receive FIFO is greater than this value + 1. Lower values mean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field description in the SSI Databook says "equal to or above this field value + 1", so:
valid data entries in the receive FIFO is greater than this value + 1. Lower values mean | |
valid data entries in the receive FIFO is greater than this value. Lower values mean |
When in DMA mode, the receive data level field controls the level at which a DMA request | ||
is made by the receive logic. A request to receive is generated when the number of | ||
valid data entries in the receive FIFO is greater than this value + 1. Lower values mean | ||
more frequent DMA triggers and higher values mean larger less frequent bursts. Range: 1-15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the range should be 0-15, for both these properties.
#define DMACR_RDMAE_BIT BIT(0) | ||
|
||
/* DMATDLR - DMA Transmit Data Level */ | ||
#define DMA_TDLR_DMATDL_MASK GENMASK(3, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define DMA_TDLR_DMATDL_MASK GENMASK(3, 0) | |
#define DMATDLR_DMATDL_MASK GENMASK(3, 0) |
void *dma_transfer_list; | ||
void *dma_joblist; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not introduce instead a generic vendor_specific_data
pointer (together with VENDOR_SPECIFIC_DATA_DEFINE()
and VENDOR_SPECIFIC_DATA_GET()
macros) that would be available always, not only when DMA is used? This would be more flexible.
if (!transfer_list) { | ||
LOG_ERR("DMA transfer list not available"); | ||
return -ENODEV; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this list be not available? It is now allocated statically.
#define MSPI_DW_DMA_XFER_LIST_GET(inst) &mspi_dw_##inst##_transfer_list | ||
#define MSPI_DW_DMA_JOBLIST_GET(inst) &mspi_dw_##inst##_joblist | ||
|
||
static inline void vendor_specific_start_dma_xfer(const struct device *dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually beneficial to have three separate functions: vendor_specific_start_dma_xfer()
, vendor_specific_setup_dma_xfer()
, and vendor_specific_dma_accessible_check()
, instead of just one that would do all that is needed to start the DMA transfer?
1 * RX_FIFO_DEPTH(inst) / 8 - 1) | ||
|
||
1 * RX_FIFO_DEPTH(inst) / 8 - 1) \ | ||
IF_ENABLED(CONFIG_MSPI_DMA, (, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite weird. I think it would be better to use a separate macro for DMA data levels that would be called from IF_ENABLED(CONFIG_MSPI_DMA, )
.
} | ||
#if defined(CONFIG_MSPI_DMA) | ||
else if (dev_data->xfer.xfer_mode == MSPI_DMA) { | ||
imr = IMR_RXFIM_BIT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually needed? The SSI interrupts don't seem to be used for DMA transfers.
PR for the mspi_dw driver to support Async and DMA transactions and support for the new nrf-mspi peripheral (name tbc). This peripheral is similar to the exmif in nRF54H20, using the same core designware IP, but has been configured to support DMA and slave mode. The DMA being used is part of the wrapper and not the core IP.
Concurrent Aync transactions and XiP support for nrf-mspi will be added in a future PR. The DMA support uses the DMA in a vendor specific wrapper.