From 1ac6f57671243338e3217869cdb5ca667d50366b Mon Sep 17 00:00:00 2001 From: Iuliana Prodan Date: Tue, 16 Feb 2021 12:38:40 +0200 Subject: [PATCH 1/4] dai: correct the order for DAI and DMA start/stop To stop/suspend an active DMA channel: 1. Stop the DMA service request at the peripheral first (stop the DAI); 2. Disable the hardware service request on the appropriate DMA channel. For start/resume: 1. Enable the DMA service request on the appropriate channel; 2. Enable the DMA service request at the peripheral (enable DAI). When the start/stop order for DMA and DAI is different, on multiple start/stop runs for playback or record or combined, we get an underrun/overflow. That's because the DAI makes a DMA request, before the DMA channel is enabled. Some platforms cannot just simple disable DMA channel during the transfer, because it will hang the whole DMA controller. Therefore, for DMA_SUSPEND_DRAIN, stop the DMA first and let the DAI drain the FIFO in order to stop the channel as soon as possible. Fixes: #3809 Signed-off-by: Iuliana Prodan --- src/audio/dai.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/audio/dai.c b/src/audio/dai.c index 45e19e0f2166..d12d68cbc4cd 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -651,11 +651,11 @@ static int dai_comp_trigger_internal(struct comp_dev *dev, int cmd) /* only start the DAI if we are not XRUN handling */ if (dd->xrun == 0) { - /* start the DAI */ - dai_trigger(dd->dai, cmd, dev->direction); ret = dma_start(dd->chan); if (ret < 0) return ret; + /* start the DAI */ + dai_trigger(dd->dai, cmd, dev->direction); } else { dd->xrun = 0; } @@ -695,8 +695,21 @@ static int dai_comp_trigger_internal(struct comp_dev *dev, int cmd) COMPILER_FALLTHROUGH; case COMP_TRIGGER_STOP: comp_dbg(dev, "dai_comp_trigger_internal(), STOP"); +/* + * Some platforms cannot just simple disable + * DMA channel during the transfer, + * because it will hang the whole DMA controller. + * Therefore, stop the DMA first and let the DAI + * drain the FIFO in order to stop the channel + * as soon as possible. + */ +#if CONFIG_DMA_SUSPEND_DRAIN ret = dma_stop(dd->chan); dai_trigger(dd->dai, cmd, dev->direction); +#else + dai_trigger(dd->dai, cmd, dev->direction); + ret = dma_stop(dd->chan); +#endif break; case COMP_TRIGGER_PAUSE: comp_dbg(dev, "dai_comp_trigger_internal(), PAUSE"); From a511165fc067633c0c3e5338389faf2726def17f Mon Sep 17 00:00:00 2001 From: Iuliana Prodan Date: Thu, 4 Mar 2021 13:01:59 +0200 Subject: [PATCH 2/4] drivers: imx: sai: set SAI watermark only once Set SAI watermark only once, on sai_set_config(). There is no need to set it each time, on start(). SAI watermark is kept on half FIFO size. Signed-off-by: Iuliana Prodan --- src/drivers/imx/sai.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/drivers/imx/sai.c b/src/drivers/imx/sai.c index 76a225d742d2..a8d10067ad11 100644 --- a/src/drivers/imx/sai.c +++ b/src/drivers/imx/sai.c @@ -50,9 +50,7 @@ static void sai_start(struct dai *dai, int direction) /* transmitter enable */ dai_update_bits(dai, REG_SAI_XCSR(direction), REG_SAI_CSR_TERE, REG_SAI_CSR_TERE); - /* TODO: for the time being use half FIFO size as watermark */ - dai_update_bits(dai, REG_SAI_XCR1(direction), - REG_SAI_CR1_RFW_MASK, SAI_FIFO_WORD_SIZE / 2); + dai_update_bits(dai, REG_SAI_XCR3(direction), REG_SAI_CR3_TRCE_MASK, REG_SAI_CR3_TRCE(1)); @@ -255,6 +253,9 @@ static inline int sai_set_config(struct dai *dai, mask_cr5 = REG_SAI_CR5_WNW_MASK | REG_SAI_CR5_W0W_MASK | REG_SAI_CR5_FBT_MASK; + /* TODO: for the time being use half FIFO size as watermark */ + dai_update_bits(dai, REG_SAI_XCR1(REG_TX_DIR), + REG_SAI_CR1_RFW_MASK, SAI_FIFO_WORD_SIZE / 2); dai_update_bits(dai, REG_SAI_XCR2(REG_TX_DIR), mask_cr2, val_cr2); dai_update_bits(dai, REG_SAI_XCR4(REG_TX_DIR), mask_cr4, val_cr4); dai_update_bits(dai, REG_SAI_XCR5(REG_TX_DIR), mask_cr5, val_cr5); @@ -264,6 +265,10 @@ static inline int sai_set_config(struct dai *dai, val_cr2 |= REG_SAI_CR2_SYNC; mask_cr2 |= REG_SAI_CR2_SYNC_MASK; + + /* TODO: for the time being use half FIFO size as watermark */ + dai_update_bits(dai, REG_SAI_XCR1(REG_RX_DIR), + REG_SAI_CR1_RFW_MASK, SAI_FIFO_WORD_SIZE / 2); dai_update_bits(dai, REG_SAI_XCR2(REG_RX_DIR), mask_cr2, val_cr2); dai_update_bits(dai, REG_SAI_XCR4(REG_RX_DIR), mask_cr4, val_cr4); dai_update_bits(dai, REG_SAI_XCR5(REG_RX_DIR), mask_cr5, val_cr5); From 8468cf283d460d77ef349904d4b255b076fd2a86 Mon Sep 17 00:00:00 2001 From: Iuliana Prodan Date: Thu, 4 Mar 2021 13:13:27 +0200 Subject: [PATCH 3/4] drivers: imx: sai: update start/stop operations For SAI, we have the synchronous mode enabled: the transmitter is configured for asynchronous operation and the receiver for synchronous operation. In this case, transmitter bit clock and frame sync are used by both the transmitter and receiver. So, when enabling RX we need to enable TX (if not already enabled). Therefore, for a clear start, we first do a software reset for the current direction, but checking the state of RX and TX. This will reset the internal transmitter/receiver logic including the FIFO pointers. For capture we can disable the receiver data channel, but on playback, we can disable the transmitter only if the RX has the DMA requests disabled. Also, for capture, there's no need to enable the transmit data channel. It's sufficient to enable only the transmitter, which enables the bit clock (shared with RX). On stop, we just need to disable the DMA request, the transmit/receive data channel, the interrupts and the receiver and/or the transmitter. Fixes: #3809 Signed-off-by: Iuliana Prodan --- src/drivers/imx/sai.c | 110 ++++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 41 deletions(-) diff --git a/src/drivers/imx/sai.c b/src/drivers/imx/sai.c index a8d10067ad11..28f97c36fe2a 100644 --- a/src/drivers/imx/sai.c +++ b/src/drivers/imx/sai.c @@ -33,13 +33,38 @@ static void sai_start(struct dai *dai, int direction) uint32_t xcsr = 0U; - /* enable DMA requests */ - dai_update_bits(dai, REG_SAI_XCSR(direction), - REG_SAI_CSR_FRDE, REG_SAI_CSR_FRDE); -#ifdef CONFIG_IMX8M - dai_update_bits(dai, REG_SAI_MCTL, REG_SAI_MCTL_MCLK_EN, - REG_SAI_MCTL_MCLK_EN); -#endif + if (direction == DAI_DIR_CAPTURE) { + /* Software Reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_CAPTURE), + REG_SAI_CSR_SR, REG_SAI_CSR_SR); + /* Clear SR bit to finish the reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_CAPTURE), + REG_SAI_CSR_SR, 0U); + /* Check if the opposite direction is also disabled */ + xcsr = dai_read(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK)); + if (!(xcsr & REG_SAI_CSR_FRDE)) { + /* Software Reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_SR, REG_SAI_CSR_SR); + /* Clear SR bit to finish the reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_SR, 0U); + /* Transmitter enable */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_TERE, REG_SAI_CSR_TERE); + } + } else { + /* Check if the opposite direction is also disabled */ + xcsr = dai_read(dai, REG_SAI_XCSR(DAI_DIR_CAPTURE)); + if (!(xcsr & REG_SAI_CSR_FRDE)) { + /* Software Reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_SR, REG_SAI_CSR_SR); + /* Clear SR bit to finish the reset */ + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_SR, 0U); + } + } /* add one word to FIFO before TRCE is enabled */ if (direction == DAI_DIR_PLAYBACK) @@ -47,25 +72,21 @@ static void sai_start(struct dai *dai, int direction) else dai_write(dai, REG_SAI_RDR0, 0x0); - /* transmitter enable */ + /* enable DMA requests */ dai_update_bits(dai, REG_SAI_XCSR(direction), - REG_SAI_CSR_TERE, REG_SAI_CSR_TERE); + REG_SAI_CSR_FRDE, REG_SAI_CSR_FRDE); +#ifdef CONFIG_IMX8M + dai_update_bits(dai, REG_SAI_MCTL, REG_SAI_MCTL_MCLK_EN, + REG_SAI_MCTL_MCLK_EN); +#endif + /* transmit/receive data channel enable */ dai_update_bits(dai, REG_SAI_XCR3(direction), REG_SAI_CR3_TRCE_MASK, REG_SAI_CR3_TRCE(1)); - if (direction == DAI_DIR_CAPTURE) { - xcsr = dai_read(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK)); - if (!(xcsr & REG_SAI_CSR_FRDE)) { - - dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), - REG_SAI_CSR_TERE, REG_SAI_CSR_TERE); - dai_update_bits(dai, REG_SAI_XCR3(DAI_DIR_PLAYBACK), - REG_SAI_CR3_TRCE_MASK, - REG_SAI_CR3_TRCE(1)); - } - } - + /* transmitter/receiver enable */ + dai_update_bits(dai, REG_SAI_XCSR(direction), + REG_SAI_CSR_TERE, REG_SAI_CSR_TERE); } static void sai_stop(struct dai *dai, int direction) @@ -74,32 +95,39 @@ static void sai_stop(struct dai *dai, int direction) uint32_t xcsr = 0U; + /* Disable DMA request */ dai_update_bits(dai, REG_SAI_XCSR(direction), REG_SAI_CSR_FRDE, 0); + + /* Transmit/Receive data channel disable */ + dai_update_bits(dai, REG_SAI_XCR3(direction), + REG_SAI_CR3_TRCE_MASK, + REG_SAI_CR3_TRCE(0)); + + /* Disable interrupts */ dai_update_bits(dai, REG_SAI_XCSR(direction), REG_SAI_CSR_XIE_MASK, 0); - /* Check if the opposite direction is also disabled */ - xcsr = dai_read(dai, REG_SAI_XCSR(!direction)); - if (!(xcsr & REG_SAI_CSR_FRDE)) { - /* Disable both directions and reset their FIFOs */ - dai_update_bits(dai, REG_SAI_TCSR, REG_SAI_CSR_TERE, 0); - poll_for_register_delay(dai_base(dai) + REG_SAI_TCSR, - REG_SAI_CSR_TERE, 0, 100); - - dai_update_bits(dai, REG_SAI_RCSR, REG_SAI_CSR_TERE, 0); - poll_for_register_delay(dai_base(dai) + REG_SAI_RCSR, + /* Disable transmitter/receiver */ + if (direction == DAI_DIR_CAPTURE) { + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_CAPTURE), REG_SAI_CSR_TERE, 0); + poll_for_register_delay(dai_base(dai) + REG_SAI_XCSR(DAI_DIR_CAPTURE), REG_SAI_CSR_TERE, 0, 100); - - /* Software Reset for both Tx and Rx */ - dai_update_bits(dai, REG_SAI_TCSR, REG_SAI_CSR_SR, - REG_SAI_CSR_SR); - dai_update_bits(dai, REG_SAI_RCSR, REG_SAI_CSR_SR, - REG_SAI_CSR_SR); - - /* Clear SR bit to finish the reset */ - dai_update_bits(dai, REG_SAI_TCSR, REG_SAI_CSR_SR, 0U); - dai_update_bits(dai, REG_SAI_RCSR, REG_SAI_CSR_SR, 0U); + /* Check if the opposite direction is also disabled */ + xcsr = dai_read(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK)); + if (!(xcsr & REG_SAI_CSR_FRDE)) { + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), REG_SAI_CSR_TERE, 0); + poll_for_register_delay(dai_base(dai) + REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_TERE, 0, 100); + } + } else { + /* Check if the opposite direction is also disabled */ + xcsr = dai_read(dai, REG_SAI_XCSR(DAI_DIR_CAPTURE)); + if (!(xcsr & REG_SAI_CSR_FRDE)) { + dai_update_bits(dai, REG_SAI_XCSR(DAI_DIR_PLAYBACK), REG_SAI_CSR_TERE, 0); + poll_for_register_delay(dai_base(dai) + REG_SAI_XCSR(DAI_DIR_PLAYBACK), + REG_SAI_CSR_TERE, 0, 100); + } } } From 852840c9d851642ae338d7a7beb01ba531b359e0 Mon Sep 17 00:00:00 2001 From: Iuliana Prodan Date: Fri, 5 Mar 2021 19:06:37 +0200 Subject: [PATCH 4/4] drivers: imx: sai: W1C for a clean start On start W1C the Work Start Flag, Sync Error Flag and FIFO Error Flag. Write a logic 1 to this field to clear each of this flags and have a clean start for SAI. Signed-off-by: Iuliana Prodan --- src/drivers/imx/sai.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/drivers/imx/sai.c b/src/drivers/imx/sai.c index 28f97c36fe2a..6ea552106c94 100644 --- a/src/drivers/imx/sai.c +++ b/src/drivers/imx/sai.c @@ -66,6 +66,14 @@ static void sai_start(struct dai *dai, int direction) } } + /* W1C */ + dai_update_bits(dai, REG_SAI_XCSR(direction), + REG_SAI_CSR_FEF, 1); + dai_update_bits(dai, REG_SAI_XCSR(direction), + REG_SAI_CSR_SEF, 1); + dai_update_bits(dai, REG_SAI_XCSR(direction), + REG_SAI_CSR_WSF, 1); + /* add one word to FIFO before TRCE is enabled */ if (direction == DAI_DIR_PLAYBACK) dai_write(dai, REG_SAI_TDR0, 0x0);