Skip to content

LPSPI optimizations 5/19/25 #90182

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jun 18, 2025
Merged

LPSPI optimizations 5/19/25 #90182

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
50000e1
spi_nxp_lpspi: Remove dev pointer from data struct
decsny May 19, 2025
bc34d1c
spi_nxp_lpspi: Clarify configuration function
decsny May 19, 2025
abd21b9
spi_nxp_lpspi: Add version ID to data struct
decsny May 19, 2025
0197baf
spi_nxp_lpspi: Add maximum wait time of fifo empty
decsny May 19, 2025
1677ec1
spi_nxp_lpspi: Remove call to MasterInit
decsny May 19, 2025
4a207d2
spi_nxp_lpspi: Remove MCUX branding
decsny May 20, 2025
ddced0a
spi_nxp_lpspi: Simplify tx ctx update
decsny Jun 3, 2025
7fb9c6f
spi_nxp_lpspi: Fix fifo fill logic
decsny Jun 3, 2025
081cdf7
spi_nxp_lpspi: Fix unit of buf_len in fill_tx_fifo
decsny Jun 10, 2025
ccd7222
spi_nxp_lpspi: Fix extra byte sent on v1 lpspi
decsny Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions drivers/spi/spi_nxp_lpspi/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,17 @@ config SPI_MCUX_LPSPI_CPU
This has lower latency than DMA-based driver but over the
longer transfers will likely have less bandwidth and use more CPU time.

config SPI_NXP_LPSPI_TXFIFO_WAIT_CYCLES
int "Number of CPU cycles to wait on TX fifo empty"
default 0 if DEBUG
default 10000
Comment on lines +41 to +42

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@decsny now zephyr project enables copilot review, but copilot is not good at Kconfig staff.

help
This option most likely does not need changed.
The drivers tend to need to wait on confirming the transmit command
is consumed by the hardware by checking of the TX fifo is emptied.
This option gives a maximum number of CPU cycles to wait on that check.
The special value of 0 means infinite, which can be useful for debugging
for if there is some programming error that causes TX fifo not to empty.
The default of 10000 is arbitrary.

endif # SPI_MCUX_LPSPI
9 changes: 6 additions & 3 deletions drivers/spi/spi_nxp_lpspi/spi_nxp_lpspi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ static inline void lpspi_handle_tx_irq(const struct device *dev)
lpspi_data->fill_len -= this_buf_words_sent;
}

lpspi_next_tx_fill(data->dev);
lpspi_next_tx_fill(dev);
}

static inline void lpspi_end_xfer(const struct device *dev)
Expand Down Expand Up @@ -276,7 +276,7 @@ static void lpspi_isr(const struct device *dev)
}

if ((DIV_ROUND_UP(spi_context_rx_len_left(ctx, word_size_bytes), word_size_bytes) == 1) &&
(LPSPI_VERID_MAJOR(base->VERID) < 2)) {
(data->major_version < 2)) {
/* Due to stalling behavior on older LPSPI,
* need to end xfer in order to get last bit clocked out on bus.
*/
Expand Down Expand Up @@ -337,7 +337,10 @@ static int transceive(const struct device *dev, const struct spi_config *spi_cfg
base->TCR |= LPSPI_TCR_CONT_MASK;
}
/* tcr is written to tx fifo */
lpspi_wait_tx_fifo_empty(dev);
ret = lpspi_wait_tx_fifo_empty(dev);
if (ret) {
return ret;
}

/* start the transfer sequence which are handled by irqs */
lpspi_next_tx_fill(dev);
Expand Down
58 changes: 37 additions & 21 deletions drivers/spi/spi_nxp_lpspi/spi_nxp_lpspi_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@
* SPDX-License-Identifier: Apache-2.0
*/

/*
* This is a collection of functions that would be useful for any driver for LPSPI.
* This function/file has no knowledge of lpspi usage model by the driver
* beyond basic configuration and should avoid making any assumptions about how
* the driver is going to achieve the zephyr API.
*/

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(spi_lpspi, CONFIG_SPI_LOG_LEVEL);

Expand Down Expand Up @@ -53,12 +60,24 @@ static inline clock_ip_name_t lpspi_get_clock(LPSPI_Type *const base)
}
#endif

void lpspi_wait_tx_fifo_empty(const struct device *dev)
int lpspi_wait_tx_fifo_empty(const struct device *dev)
{
LPSPI_Type *base = (LPSPI_Type *)DEVICE_MMIO_NAMED_GET(dev, reg_base);
int arbitrary_cycle_limit = CONFIG_SPI_NXP_LPSPI_TXFIFO_WAIT_CYCLES;
bool limit_wait = arbitrary_cycle_limit > 0;

while (FIELD_GET(LPSPI_FSR_TXCOUNT_MASK, base->FSR) != 0) {
if (!limit_wait) {
continue;
}

while (LPSPI_GetTxFifoCount(base) != 0) {
if (arbitrary_cycle_limit-- < 0) {
LOG_WRN("Failed waiting for TX fifo empty");
return -EIO;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use k_busy_wait() here for maybe 1usec per iteration and use the Kconfig to determine total the wait time in usec. This way the behavior will not vary by the processor speed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's supposed to vary by processor speed. It's just a number of attempts to try before quitting. It's simply a basic safety mechanism, which is just about being more safe than sorry, not a unique feature. It shouldn't even be needed. Most likely this consumption by the fifo happens immediately, and this loop isn't necessary. But people wanted to expand the scope of this PR, so here it is. I'm not overcomplicating it any more by making it time -based based on CPU frequency calculations.

Also the specific suggestion to use k_busy_wait is not a good idea. That function is not isr safe. This function is called in isr context. k_busy_wait is implemented by polling kernel time, which can't increment due to this function would be blocking in isr context, causing a likely infinite loop in the kernel.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is polling a register waiting for some truth expression with a timeout, WAIT_FOR is the appropriate tool. It uses cycles not ticks while accepting us timeouts. If you see any bugs in this do send fixes.

https://docs.zephyrproject.org/apidoc/latest/group__sys-util.html#ga68eb35df6b4715714312df90209accee

}

return 0;
}

int spi_lpspi_release(const struct device *dev, const struct spi_config *spi_cfg)
Expand Down Expand Up @@ -131,23 +150,20 @@ int spi_mcux_configure(const struct device *dev, const struct spi_config *spi_cf
return ret;
}

if (already_configured) {
/* Setting the baud rate in LPSPI_MasterInit requires module to be disabled. Only
* disable if already configured, otherwise the clock is not enabled and the
* CR register cannot be written.
*/
LPSPI_Enable(base, false);
while ((base->CR & LPSPI_CR_MEN_MASK) != 0U) {
/* Wait until LPSPI is disabled. Datasheet:
* After writing 0, MEN (Module Enable) remains set until the LPSPI has
* completed the current transfer and is idle.
*/
}
/* specific driver implementation should set up watermarks and interrupts.
* we reset them here to avoid any unexpected events during configuring.
*/
base->FCR = 0;
base->IER = 0;

/* this is workaround for ERR050456 */
base->CR |= LPSPI_CR_RST_MASK;
base->CR |= LPSPI_CR_RRF_MASK | LPSPI_CR_RTF_MASK;

/* this is workaround for ERR050456 */
base->CR |= LPSPI_CR_RST_MASK;
base->CR |= LPSPI_CR_RRF_MASK | LPSPI_CR_RTF_MASK;
base->CR = 0x00U;
/* Setting the baud rate requires module to be disabled. */
base->CR = 0;
while ((base->CR & LPSPI_CR_MEN_MASK) != 0) {
/* According to datasheet, should wait for this MEN bit to clear once idle */
}

data->ctx.config = spi_cfg;
Expand Down Expand Up @@ -181,7 +197,7 @@ int spi_mcux_configure(const struct device *dev, const struct spi_config *spi_cf
base->CR |= LPSPI_CR_DBGEN_MASK;
}

return 0;
return lpspi_wait_tx_fifo_empty(dev);
}

static void lpspi_module_system_init(LPSPI_Type *base)
Expand All @@ -204,15 +220,15 @@ int spi_nxp_init_common(const struct device *dev)

DEVICE_MMIO_NAMED_MAP(dev, reg_base, K_MEM_CACHE_NONE | K_MEM_DIRECT_MAP);

data->dev = dev;

if (!device_is_ready(config->clock_dev)) {
LOG_ERR("clock control device not ready");
return -ENODEV;
}

lpspi_module_system_init(base);

data->major_version = (base->VERID & LPSPI_VERID_MAJOR_MASK) >> LPSPI_VERID_MAJOR_SHIFT;

err = spi_context_cs_configure_all(&data->ctx);
if (err < 0) {
return err;
Expand Down
4 changes: 2 additions & 2 deletions drivers/spi/spi_nxp_lpspi/spi_nxp_lpspi_priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ struct lpspi_config {

struct lpspi_data {
DEVICE_MMIO_NAMED_RAM(reg_base);
const struct device *dev;
struct spi_context ctx;
void *driver_data;
size_t transfer_len;
uint8_t major_version;
};

/* verifies spi_cfg validity and set up configuration of hardware for xfer */
Expand All @@ -68,7 +68,7 @@ int spi_nxp_init_common(const struct device *dev);
/* common api function for now */
int spi_lpspi_release(const struct device *dev, const struct spi_config *spi_cfg);

void lpspi_wait_tx_fifo_empty(const struct device *dev);
int lpspi_wait_tx_fifo_empty(const struct device *dev);

#define SPI_LPSPI_IRQ_FUNC_LP_FLEXCOMM(n) \
nxp_lp_flexcomm_setirqhandler(DEVICE_DT_GET(DT_INST_PARENT(n)), DEVICE_DT_INST_GET(n), \
Expand Down