Re: [PATCH v2 06/16] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
> and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
> later calls another fsl_ssi_rxtx_config().
> 
> However, the whole routine, especially fsl_ssi_config() function,
> is too complicated because of the folowing reasons:
> 1) It has to handle the concern of the opposite stream.
> 2) It has to handle cases of offline configurations support.
> 3) It has to handle enable and disable operations while they're
>mostly different.
> 
> Since the enable and disable routines have more differences than
> TX and RX rountines, this patch simplifies these helper functions
> with the following changes:
> - Changing to two helper functions of enable and disable instead
>   of TX and RX.
> - Removing fsl_ssi_rxtx_config() by separately integrating it to
>   two newly introduced enable & disable functions.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 256 
> +++-
>  1 file changed, 122 insertions(+), 134 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 263c067..09a571a 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -378,31 +378,83 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
>  }
>  
>  /**
> - * Enable or disable all rx/tx config flags at once
> + * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
> + *
> + * Notes:
> + * 1) For offline_config SoCs, enable all necessary bits of both streams
> + *when 1st stream starts, even if the opposite stream will not start
> + * 2) It also clears FIFO before setting regvals; SOR is safe to set online
>   */
> -static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
> +static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
>  {
> - struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *vals = ssi->regvals;
> + bool dir = tx ? TX : RX;

A similar case as in patch 3 of a bool variable used for a bit index.

> + u32 sier, srcr, stcr;
>  
> - if (enable) {
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier,
> -vals[RX].sier | vals[TX].sier);
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr,
> -vals[RX].srcr | vals[TX].srcr);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr,
> -vals[RX].stcr | vals[TX].stcr);
> + /* Clear dirty data in the FIFO; It also prevents channel slipping */
> + regmap_update_bits(ssi->regs, REG_SSI_SOR,
> +SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
> +
> + /*
> +  * On offline_config SoCs, SxCR and SIER are already configured when
> +  * the previous stream started. So skip all SxCR and SIER settings
> +  * to prevent online reconfigurations, then jump to set SCR directly
> +  */
> + if (ssi->soc->offline_config && ssi->streams)
> + goto enable_scr;
> +
> + if (ssi->soc->offline_config) {
> + /*
> +  * Online reconfiguration not supported, so enable all bits for
> +  * both streams at once to avoid necessity of reconfigurations
> +  */
> + srcr = vals[RX].srcr | vals[TX].srcr;
> + stcr = vals[RX].stcr | vals[TX].stcr;
> + sier = vals[RX].sier | vals[TX].sier;
>   } else {
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr, 0);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr, 0);
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier, 0);
> + /* Otherwise, only set bits for the current stream */
> + srcr = vals[dir].srcr;
> + stcr = vals[dir].stcr;
> + sier = vals[dir].sier;
>   }
> +
> + /* Configure SRCR, STCR and SIER at once */
> + regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
> + regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
> + regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
> +
> +enable_scr:
> + /*
> +  * Start DMA before setting TE to avoid FIFO underrun
> +  * which may cause a channel slip or a channel swap
> +  *
> +  * TODO: FIQ cases might also need this upon testing
> +  */
> + if (ssi->use_dma && tx) {
> + int try = 100;
> + u32 sfcsr;
> +
> + /* Enable SSI first to send TX DMA request */
> + 

Re: [PATCH v2 06/16] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
> and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
> later calls another fsl_ssi_rxtx_config().
> 
> However, the whole routine, especially fsl_ssi_config() function,
> is too complicated because of the folowing reasons:
> 1) It has to handle the concern of the opposite stream.
> 2) It has to handle cases of offline configurations support.
> 3) It has to handle enable and disable operations while they're
>mostly different.
> 
> Since the enable and disable routines have more differences than
> TX and RX rountines, this patch simplifies these helper functions
> with the following changes:
> - Changing to two helper functions of enable and disable instead
>   of TX and RX.
> - Removing fsl_ssi_rxtx_config() by separately integrating it to
>   two newly introduced enable & disable functions.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 256 
> +++-
>  1 file changed, 122 insertions(+), 134 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 263c067..09a571a 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -378,31 +378,83 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
>  }
>  
>  /**
> - * Enable or disable all rx/tx config flags at once
> + * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
> + *
> + * Notes:
> + * 1) For offline_config SoCs, enable all necessary bits of both streams
> + *when 1st stream starts, even if the opposite stream will not start
> + * 2) It also clears FIFO before setting regvals; SOR is safe to set online
>   */
> -static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
> +static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
>  {
> - struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *vals = ssi->regvals;
> + bool dir = tx ? TX : RX;

A similar case as in patch 3 of a bool variable used for a bit index.

> + u32 sier, srcr, stcr;
>  
> - if (enable) {
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier,
> -vals[RX].sier | vals[TX].sier);
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr,
> -vals[RX].srcr | vals[TX].srcr);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr,
> -vals[RX].stcr | vals[TX].stcr);
> + /* Clear dirty data in the FIFO; It also prevents channel slipping */
> + regmap_update_bits(ssi->regs, REG_SSI_SOR,
> +SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
> +
> + /*
> +  * On offline_config SoCs, SxCR and SIER are already configured when
> +  * the previous stream started. So skip all SxCR and SIER settings
> +  * to prevent online reconfigurations, then jump to set SCR directly
> +  */
> + if (ssi->soc->offline_config && ssi->streams)
> + goto enable_scr;
> +
> + if (ssi->soc->offline_config) {
> + /*
> +  * Online reconfiguration not supported, so enable all bits for
> +  * both streams at once to avoid necessity of reconfigurations
> +  */
> + srcr = vals[RX].srcr | vals[TX].srcr;
> + stcr = vals[RX].stcr | vals[TX].stcr;
> + sier = vals[RX].sier | vals[TX].sier;
>   } else {
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr, 0);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr, 0);
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier, 0);
> + /* Otherwise, only set bits for the current stream */
> + srcr = vals[dir].srcr;
> + stcr = vals[dir].stcr;
> + sier = vals[dir].sier;
>   }
> +
> + /* Configure SRCR, STCR and SIER at once */
> + regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
> + regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
> + regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
> +
> +enable_scr:
> + /*
> +  * Start DMA before setting TE to avoid FIFO underrun
> +  * which may cause a channel slip or a channel swap
> +  *
> +  * TODO: FIQ cases might also need this upon testing
> +  */
> + if (ssi->use_dma && tx) {
> + int try = 100;
> + u32 sfcsr;
> +
> + /* Enable SSI first to send TX DMA request */
> + regmap_update_bits(ssi->regs, REG_SSI_SCR,
> + 

[PATCH v2 06/16] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Nicolin Chen]

The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
later calls another fsl_ssi_rxtx_config().

However, the whole routine, especially fsl_ssi_config() function,
is too complicated because of the folowing reasons:
1) It has to handle the concern of the opposite stream.
2) It has to handle cases of offline configurations support.
3) It has to handle enable and disable operations while they're
   mostly different.

Since the enable and disable routines have more differences than
TX and RX rountines, this patch simplifies these helper functions
with the following changes:
- Changing to two helper functions of enable and disable instead
  of TX and RX.
- Removing fsl_ssi_rxtx_config() by separately integrating it to
  two newly introduced enable & disable functions.

Signed-off-by: Nicolin Chen 
Tested-by: Caleb Crome 
---
 sound/soc/fsl/fsl_ssi.c | 256 +++-
 1 file changed, 122 insertions(+), 134 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 263c067..09a571a 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -378,31 +378,83 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 }
 
 /**
- * Enable or disable all rx/tx config flags at once
+ * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
+ *
+ * Notes:
+ * 1) For offline_config SoCs, enable all necessary bits of both streams
+ *when 1st stream starts, even if the opposite stream will not start
+ * 2) It also clears FIFO before setting regvals; SOR is safe to set online
  */
-static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
+static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
 {
-   struct regmap *regs = ssi->regs;
struct fsl_ssi_regvals *vals = ssi->regvals;
+   bool dir = tx ? TX : RX;
+   u32 sier, srcr, stcr;
 
-   if (enable) {
-   regmap_update_bits(regs, REG_SSI_SIER,
-  vals[RX].sier | vals[TX].sier,
-  vals[RX].sier | vals[TX].sier);
-   regmap_update_bits(regs, REG_SSI_SRCR,
-  vals[RX].srcr | vals[TX].srcr,
-  vals[RX].srcr | vals[TX].srcr);
-   regmap_update_bits(regs, REG_SSI_STCR,
-  vals[RX].stcr | vals[TX].stcr,
-  vals[RX].stcr | vals[TX].stcr);
+   /* Clear dirty data in the FIFO; It also prevents channel slipping */
+   regmap_update_bits(ssi->regs, REG_SSI_SOR,
+  SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
+
+   /*
+* On offline_config SoCs, SxCR and SIER are already configured when
+* the previous stream started. So skip all SxCR and SIER settings
+* to prevent online reconfigurations, then jump to set SCR directly
+*/
+   if (ssi->soc->offline_config && ssi->streams)
+   goto enable_scr;
+
+   if (ssi->soc->offline_config) {
+   /*
+* Online reconfiguration not supported, so enable all bits for
+* both streams at once to avoid necessity of reconfigurations
+*/
+   srcr = vals[RX].srcr | vals[TX].srcr;
+   stcr = vals[RX].stcr | vals[TX].stcr;
+   sier = vals[RX].sier | vals[TX].sier;
} else {
-   regmap_update_bits(regs, REG_SSI_SRCR,
-  vals[RX].srcr | vals[TX].srcr, 0);
-   regmap_update_bits(regs, REG_SSI_STCR,
-  vals[RX].stcr | vals[TX].stcr, 0);
-   regmap_update_bits(regs, REG_SSI_SIER,
-  vals[RX].sier | vals[TX].sier, 0);
+   /* Otherwise, only set bits for the current stream */
+   srcr = vals[dir].srcr;
+   stcr = vals[dir].stcr;
+   sier = vals[dir].sier;
}
+
+   /* Configure SRCR, STCR and SIER at once */
+   regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
+   regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
+   regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
+
+enable_scr:
+   /*
+* Start DMA before setting TE to avoid FIFO underrun
+* which may cause a channel slip or a channel swap
+*
+* TODO: FIQ cases might also need this upon testing
+*/
+   if (ssi->use_dma && tx) {
+   int try = 100;
+   u32 sfcsr;
+
+   /* Enable SSI first to send TX DMA request */
+   regmap_update_bits(ssi->regs, REG_SSI_SCR,
+  SSI_SCR_SSIEN, SSI_SCR_SSIEN);
+
+   /* Busy wait until TX FIFO not empty -- DMA working */
+   do {
+   

[PATCH v2 06/16] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Nicolin Chen]

The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
later calls another fsl_ssi_rxtx_config().

However, the whole routine, especially fsl_ssi_config() function,
is too complicated because of the folowing reasons:
1) It has to handle the concern of the opposite stream.
2) It has to handle cases of offline configurations support.
3) It has to handle enable and disable operations while they're
   mostly different.

Since the enable and disable routines have more differences than
TX and RX rountines, this patch simplifies these helper functions
with the following changes:
- Changing to two helper functions of enable and disable instead
  of TX and RX.
- Removing fsl_ssi_rxtx_config() by separately integrating it to
  two newly introduced enable & disable functions.

Signed-off-by: Nicolin Chen 
Tested-by: Caleb Crome 
---
 sound/soc/fsl/fsl_ssi.c | 256 +++-
 1 file changed, 122 insertions(+), 134 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 263c067..09a571a 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -378,31 +378,83 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 }
 
 /**
- * Enable or disable all rx/tx config flags at once
+ * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
+ *
+ * Notes:
+ * 1) For offline_config SoCs, enable all necessary bits of both streams
+ *when 1st stream starts, even if the opposite stream will not start
+ * 2) It also clears FIFO before setting regvals; SOR is safe to set online
  */
-static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
+static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
 {
-   struct regmap *regs = ssi->regs;
struct fsl_ssi_regvals *vals = ssi->regvals;
+   bool dir = tx ? TX : RX;
+   u32 sier, srcr, stcr;
 
-   if (enable) {
-   regmap_update_bits(regs, REG_SSI_SIER,
-  vals[RX].sier | vals[TX].sier,
-  vals[RX].sier | vals[TX].sier);
-   regmap_update_bits(regs, REG_SSI_SRCR,
-  vals[RX].srcr | vals[TX].srcr,
-  vals[RX].srcr | vals[TX].srcr);
-   regmap_update_bits(regs, REG_SSI_STCR,
-  vals[RX].stcr | vals[TX].stcr,
-  vals[RX].stcr | vals[TX].stcr);
+   /* Clear dirty data in the FIFO; It also prevents channel slipping */
+   regmap_update_bits(ssi->regs, REG_SSI_SOR,
+  SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
+
+   /*
+* On offline_config SoCs, SxCR and SIER are already configured when
+* the previous stream started. So skip all SxCR and SIER settings
+* to prevent online reconfigurations, then jump to set SCR directly
+*/
+   if (ssi->soc->offline_config && ssi->streams)
+   goto enable_scr;
+
+   if (ssi->soc->offline_config) {
+   /*
+* Online reconfiguration not supported, so enable all bits for
+* both streams at once to avoid necessity of reconfigurations
+*/
+   srcr = vals[RX].srcr | vals[TX].srcr;
+   stcr = vals[RX].stcr | vals[TX].stcr;
+   sier = vals[RX].sier | vals[TX].sier;
} else {
-   regmap_update_bits(regs, REG_SSI_SRCR,
-  vals[RX].srcr | vals[TX].srcr, 0);
-   regmap_update_bits(regs, REG_SSI_STCR,
-  vals[RX].stcr | vals[TX].stcr, 0);
-   regmap_update_bits(regs, REG_SSI_SIER,
-  vals[RX].sier | vals[TX].sier, 0);
+   /* Otherwise, only set bits for the current stream */
+   srcr = vals[dir].srcr;
+   stcr = vals[dir].stcr;
+   sier = vals[dir].sier;
}
+
+   /* Configure SRCR, STCR and SIER at once */
+   regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
+   regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
+   regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
+
+enable_scr:
+   /*
+* Start DMA before setting TE to avoid FIFO underrun
+* which may cause a channel slip or a channel swap
+*
+* TODO: FIQ cases might also need this upon testing
+*/
+   if (ssi->use_dma && tx) {
+   int try = 100;
+   u32 sfcsr;
+
+   /* Enable SSI first to send TX DMA request */
+   regmap_update_bits(ssi->regs, REG_SSI_SCR,
+  SSI_SCR_SSIEN, SSI_SCR_SSIEN);
+
+   /* Busy wait until TX FIFO not empty -- DMA working */
+   do {
+   regmap_read(ssi->regs, REG_SSI_SFCSR, );
+