intel: ssp: fix RX fifo drain logic #7711
Conversation
Separate the RX drain functions for start and stop cases. For startup, do not add any delays but read the full fifo out and ensure the dma request line (SSSR.RFS) is cleared before new capture stream is started. Link: thesofproject#7548 Fixes: 4a4d8d2 ("intel: ssp: drain RX fifo when starting") Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
| ssp_read(dai, SSDR); | ||
| } | ||
| #if CONFIG_DMA_SUSPEND_DRAIN | ||
| for (i = 0; i < entries + 1; i++) |
There was a problem hiding this comment.
can remove 106 - 108, and use:
for ( int i = 0; i < entries + 1; i++)
There was a problem hiding this comment.
Yeah, that would be nicer, but for now, we've stick to Linux kernel style and declare variable at start. I know we start to have some C++ inspired code that is not following this anymore...
| while ((ssp_read(dai, SSSR) & SSSR_RNE) && retry--) { | ||
| entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3)); | ||
|
|
||
| /* let DMA consume data or read RX FIFO directly */ |
There was a problem hiding this comment.
the comment is not correct.
| for (i = 0; i < entries + 1; i++) | ||
| ssp_read(dai, SSDR); | ||
|
|
||
| entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3)); |
There was a problem hiding this comment.
no need to re-read the entries here.
| } | ||
|
|
||
| /* clear interrupt */ | ||
| ssp_update_bits(dai, SSSR, SSSR_ROR, SSSR_ROR); |
There was a problem hiding this comment.
Clear Receive FIFO overrun
| } | ||
|
|
||
| /* empty SSP receive FIFO */ | ||
| static void ssp_empty_rx_fifo(struct dai *dai) |
There was a problem hiding this comment.
if the other incarnation is ssp_empty_rx_fifo_for_start() then this is ssp_empty_rx_fifo_for_stop()?
| #if CONFIG_DMA_SUSPEND_DRAIN | ||
| for (i = 0; i < entries + 1; i++) | ||
| ssp_read(dai, SSDR); | ||
| #endif |
There was a problem hiding this comment.
I'm not certain about this. Even if the DMA is active (not disabled) but the RX FIFO does not have trigger amount of data then the DMA will not clear the FIFO, so we just spin w/o any reason.
There was a problem hiding this comment.
@ujfalusi Ack, so this will not drain the samples that fall below the DMA trigger threshold and the function will timeout (which at 8kHz is a long timeout). OTOH, in old code fifo is emptied directly and DMA is left without trailing data, so not great either.
|
@kv2019i i've tested the patch with noraml scenario and with no-codec tplg with 8k rate. Issue is not reproducible. |
|
Excellent. thanks @Vamshigopal ! @ujfalusi is working on a revised patch we are currently stress testing on multiple devices. This builds on this same fix, but addresses some of the issues raised in this review. We'll share this later for review. |
| /* | ||
| * Empty SSP receive FIFO before DMA startup and | ||
| * ensure the DMA request is not already raised when | ||
| * DMA is started. If TX is not active, this is a no-op. |
There was a problem hiding this comment.
I'm confused a bit: isn't receive FIFO usually the FIFO where data arrives from the interface, i.e. for capture? Why is it talking about TX then?
There was a problem hiding this comment.
@lyakh #7717 is now a better version, but to clarify this bit (as this confused me initially as well). The SSP drivers run the hw in full-duplex manner. When the port is enabled with clocks running, both RX+FX fifos are running. They might not be connected to pins, but the fifos are getting cycled. The RX specifically gets filled while TX is running. So this creates specific problems when we need to start/stop the DMA for RX when TX is running concurrently.
|
Closing, let's go with #7717 instead. |
5 participants
Separate the RX drain functions for start and stop cases. For startup, do not add any delays but read the full fifo out and ensure the dma request line (SSSR.RFS) is cleared before new capture stream is started.
Link: #7548
Fixes: 4a4d8d2 ("intel: ssp: drain RX fifo when starting")