LPSPI Fix Timeout waiting for transfer complete #87548
Conversation
zephyrbot
added
area: SPI
zephyrbot
requested review from
dbaluta,
dleach02,
manuargue,
mmahadevan108,
Raymond0225,
tbursztyka and
teburd
PetervdPerk-NXP
force-pushed
the
pr-lpspi-race-fix
branch
3 times, most recently
from
15a6617 to
e5f071a
Compare
There was a problem hiding this comment.
This does cause #87144 to fail, but since it's not merged yet I can't complain, I'll debug that
The unlock code seems right, should work for slave mode since rv would be >=0 if success also.
As for the main change here which is to the isr, I think it seems right to me and I think I prefer it to the old code. It seems logically more robust because maybe there could have been a case before where there was 1 more RX left and also TX fifo was empty, so before it would not have filled a NOP due to the code to do so was conditional on the RX len not being 1, which was probably wrong. So I think this handles that case and I can't think of a problem this would cause, I think in all cases we want to handle sending TX nops if there is ever any RX left, no matter if it is 1 or otherwise. (1 being the special case due to RT stalling behavior, but you said you tested on there, so probably fine). I will probably test on RT1050 before this merges in case it has a different LPSPI version.
| @@ -153,6 +154,8 @@ int spi_nxp_init_common(const struct device *dev) | |||
| return err; | |||
| } | |||
|
|
|||
| LPSPI_Reset(base); | |||
There was a problem hiding this comment.
why was this needed ? just wondering.
There was a problem hiding this comment.
On the IMX95 we load code into ITCM and execute from there due to that there's no POR.
Thus we can't guarantee that the LPSPI in the right state on startup, so I prefer to reset a peripheral on initialization.
There was a problem hiding this comment.
ok, just FYI, I was planning to hardware reset peripheral at module level, in the PR I linked above, but we can do software peripheral reset also
There was a problem hiding this comment.
Well not every chip has a RESET_ReleasePeripheralReset interface whereas each LPSPI has a LPSPI_Reset.
So if you really want to use RESET_ReleasePeripheralReset you would have to implement a fallback anyhow.
There was a problem hiding this comment.
Yes I know, but actually the hardware signal reset is required on some platforms anyways before interfacing to the peripheral, even to do a software reset. But like I said I have no issue with this
PetervdPerk-NXP
force-pushed
the
pr-lpspi-race-fix
branch
from
e5f071a to
7c3a3f4
Compare
Fixes wait for completion problems where the ISR was not sending out TX NOP's when needed causing the transfer to timeout Signed-off-by: Peter van der Perk <peter.vanderperk@nxp.com>
Ensure that LPSPI is in POR state when initializing Signed-off-by: Peter van der Perk <peter.vanderperk@nxp.com>
When spi_mcux_dma_next_fill failed it return without releasing the lock Signed-off-by: Peter van der Perk <peter.vanderperk@nxp.com>
PetervdPerk-NXP
force-pushed
the
pr-lpspi-race-fix
branch
from
7c3a3f4 to
7aa4c56
Compare
In certain usecases SPI tranceive would trigger the following error and would be blocking the bus as well.
This PR fixes that by changing the ISR logic to ensure that on TX a NOP will be transmitted fixing this timeout race condition.
Also it checks now the output
spi_context_wait_for_completionso when a timeout occurs it will release spi lock fixing a locking issue as well.Also we reset the LPSPI peripheral now on startup to ensure the LPSPI in POR state when starting zephyr.
Tested on