* [PATCH] ASoC: fsl_esai: Support synchronous mode
@ 2019-04-01 11:39 S.j. Wang
2019-04-01 18:17 ` Nicolin Chen
0 siblings, 1 reply; 8+ messages in thread
From: S.j. Wang @ 2019-04-01 11:39 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel
Cc: linuxppc-dev
In ESAI synchronous mode, the clock is generated by Tx, So
we should always set registers of Tx which relate with the
bit clock and frame clock generation (TCCR, TCR, ECR), even
there is only Rx is working.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 3623aa9a6f2e..d9fcddd55c02 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
return -EINVAL;
}
+ if (esai_priv->synchronous && !tx) {
+ switch (clk_id) {
+ case ESAI_HCKR_FSYS:
+ fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
+ freq, dir);
+ break;
+ case ESAI_HCKR_EXTAL:
+ fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
+ freq, dir);
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
+
/* Bypass divider settings if the requirement doesn't change */
if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
return 0;
@@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx, bclk);
if (ret)
return ret;
+ if (esai_priv->synchronous && !tx) {
+ /* Use Normal mode to support monaural audio */
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
+ ESAI_xCR_xMOD_NETWORK : 0);
+
+ mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
+ val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
+ }
+
/* Use Normal mode to support monaural audio */
regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: fsl_esai: Support synchronous mode
2019-04-01 11:39 [PATCH] ASoC: fsl_esai: Support synchronous mode S.j. Wang
@ 2019-04-01 18:17 ` Nicolin Chen
2019-04-02 10:16 ` S.j. Wang
0 siblings, 1 reply; 8+ messages in thread
From: Nicolin Chen @ 2019-04-01 18:17 UTC (permalink / raw)
To: S.j. Wang; +Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
Shengjiu,
On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> In ESAI synchronous mode, the clock is generated by Tx, So
> we should always set registers of Tx which relate with the
> bit clock and frame clock generation (TCCR, TCR, ECR), even
> there is only Rx is working.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 3623aa9a6f2e..d9fcddd55c02 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> return -EINVAL;
> }
>
> + if (esai_priv->synchronous && !tx) {
> + switch (clk_id) {
> + case ESAI_HCKR_FSYS:
> + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> + freq, dir);
> + break;
> + case ESAI_HCKR_EXTAL:
> + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> + freq, dir);
Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It
feels very confusing to do so, especially without a comments.
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> /* Bypass divider settings if the requirement doesn't change */
> if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
> return 0;
> @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
>
> bclk = params_rate(params) * slot_width * esai_priv->slots;
>
> - ret = fsl_esai_set_bclk(dai, tx, bclk);
> + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx, bclk);
> if (ret)
> return ret;
>
> + if (esai_priv->synchronous && !tx) {
> + /* Use Normal mode to support monaural audio */
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
> + ESAI_xCR_xMOD_NETWORK : 0);
> +
> + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
> + }
Does synchronous mode require to set both TCR and RCR? or just TCR?
The code behind this part is doing the same setting to RCR. If that
is not needed any more for a synchronous recording, we should reuse
it instead of inserting a piece of redundant code. Otherwise, if we
need to set both, we should have two regmap_update_bits operations
back-to-back for TCR and RCR (and other registers too).
> +
> /* Use Normal mode to support monaural audio */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
In case that we only need to set TCR (more likely I feel), it would
feel less confusing to me, if we changed REG_ESAI_xCR(tx) here, for
example, to REG_ESAI_xCR(tx || sync). Yea, please add to the top a
'bool sync = esai_priv->synchronous;'.
Similarly, for ECR_ETO and ECR_ERO:
(tx || sync) ? ESAI_ECR_ETO : ESAI_ECR_ERO;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: fsl_esai: Support synchronous mode
2019-04-01 18:17 ` Nicolin Chen
@ 2019-04-02 10:16 ` S.j. Wang
0 siblings, 0 replies; 8+ messages in thread
From: S.j. Wang @ 2019-04-02 10:16 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
Hi
>
> Shengjiu,
>
> On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > In ESAI synchronous mode, the clock is generated by Tx, So we should
> > always set registers of Tx which relate with the bit clock and frame
> > clock generation (TCCR, TCR, ECR), even there is only Rx is working.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > 1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > 3623aa9a6f2e..d9fcddd55c02 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > return -EINVAL;
> > }
> >
> > + if (esai_priv->synchronous && !tx) {
> > + switch (clk_id) {
> > + case ESAI_HCKR_FSYS:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > + freq, dir);
> > + break;
> > + case ESAI_HCKR_EXTAL:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > + freq, dir);
>
> Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It feels very
> confusing to do so, especially without a comments.
For sync mode, only RX is enabled, the register of tx should be set, so call the
Set_dai_sysclk again.
>
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > /* Bypass divider settings if the requirement doesn't change */
> > if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
> > return 0;
> > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > snd_pcm_substream *substream,
> >
> > bclk = params_rate(params) * slot_width * esai_priv->slots;
> >
> > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > +bclk);
> > if (ret)
> > return ret;
> >
> > + if (esai_priv->synchronous && !tx) {
> > + /* Use Normal mode to support monaural audio */
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > + ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
> > + ESAI_xCR_xMOD_NETWORK : 0);
> > +
> > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> mask, val);
> > + }
>
> Does synchronous mode require to set both TCR and RCR? or just TCR?
> The code behind this part is doing the same setting to RCR. If that is not
> needed any more for a synchronous recording, we should reuse it instead
> of inserting a piece of redundant code. Otherwise, if we need to set both,
> we should have two regmap_update_bits operations back-to-back for TCR
> and RCR (and other registers too).
Both TCR and RCR. RCR will be set in normal flow.
>
> > +
> > /* Use Normal mode to support monaural audio */
> > regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> > ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
>
> In case that we only need to set TCR (more likely I feel), it would feel less
> confusing to me, if we changed REG_ESAI_xCR(tx) here, for example, to
> REG_ESAI_xCR(tx || sync). Yea, please add to the top a 'bool sync =
> esai_priv->synchronous;'.
>
> Similarly, for ECR_ETO and ECR_ERO:
> (tx || sync) ? ESAI_ECR_ETO : ESAI_ECR_ERO;
Both TCR and RCR should be set.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ASoC: fsl_esai: Support synchronous mode
@ 2019-04-02 10:16 ` S.j. Wang
0 siblings, 0 replies; 8+ messages in thread
From: S.j. Wang @ 2019-04-02 10:16 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
Hi
>
> Shengjiu,
>
> On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > In ESAI synchronous mode, the clock is generated by Tx, So we should
> > always set registers of Tx which relate with the bit clock and frame
> > clock generation (TCCR, TCR, ECR), even there is only Rx is working.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > 1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > 3623aa9a6f2e..d9fcddd55c02 100644
> > --- a/sound/soc/fsl/fsl_esai.c
> > +++ b/sound/soc/fsl/fsl_esai.c
> > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> snd_soc_dai *dai, int clk_id,
> > return -EINVAL;
> > }
> >
> > + if (esai_priv->synchronous && !tx) {
> > + switch (clk_id) {
> > + case ESAI_HCKR_FSYS:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > + freq, dir);
> > + break;
> > + case ESAI_HCKR_EXTAL:
> > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > + freq, dir);
>
> Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It feels very
> confusing to do so, especially without a comments.
For sync mode, only RX is enabled, the register of tx should be set, so call the
Set_dai_sysclk again.
>
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > /* Bypass divider settings if the requirement doesn't change */
> > if (freq == esai_priv->hck_rate[tx] && dir == esai_priv->hck_dir[tx])
> > return 0;
> > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > snd_pcm_substream *substream,
> >
> > bclk = params_rate(params) * slot_width * esai_priv->slots;
> >
> > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > +bclk);
> > if (ret)
> > return ret;
> >
> > + if (esai_priv->synchronous && !tx) {
> > + /* Use Normal mode to support monaural audio */
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > + ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
> > + ESAI_xCR_xMOD_NETWORK : 0);
> > +
> > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> mask, val);
> > + }
>
> Does synchronous mode require to set both TCR and RCR? or just TCR?
> The code behind this part is doing the same setting to RCR. If that is not
> needed any more for a synchronous recording, we should reuse it instead
> of inserting a piece of redundant code. Otherwise, if we need to set both,
> we should have two regmap_update_bits operations back-to-back for TCR
> and RCR (and other registers too).
Both TCR and RCR. RCR will be set in normal flow.
>
> > +
> > /* Use Normal mode to support monaural audio */
> > regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> > ESAI_xCR_xMOD_MASK,
> params_channels(params) > 1 ?
>
> In case that we only need to set TCR (more likely I feel), it would feel less
> confusing to me, if we changed REG_ESAI_xCR(tx) here, for example, to
> REG_ESAI_xCR(tx || sync). Yea, please add to the top a 'bool sync =
> esai_priv->synchronous;'.
>
> Similarly, for ECR_ETO and ECR_ERO:
> (tx || sync) ? ESAI_ECR_ETO : ESAI_ECR_ERO;
Both TCR and RCR should be set.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: fsl_esai: Support synchronous mode
2019-04-02 10:16 ` S.j. Wang
@ 2019-04-02 20:01 ` Nicolin Chen
-1 siblings, 0 replies; 8+ messages in thread
From: Nicolin Chen @ 2019-04-02 20:01 UTC (permalink / raw)
To: S.j. Wang; +Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
> > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > In ESAI synchronous mode, the clock is generated by Tx, So we should
> > > always set registers of Tx which relate with the bit clock and frame
> > > clock generation (TCCR, TCR, ECR), even there is only Rx is working.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > --- a/sound/soc/fsl/fsl_esai.c
> > > +++ b/sound/soc/fsl/fsl_esai.c
> > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > snd_soc_dai *dai, int clk_id,
> > > return -EINVAL;
> > > }
> > >
> > > + if (esai_priv->synchronous && !tx) {
> > > + switch (clk_id) {
> > > + case ESAI_HCKR_FSYS:
> > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > + freq, dir);
> > > + break;
> > > + case ESAI_HCKR_EXTAL:
> > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > + freq, dir);
> >
> > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It feels very
> > confusing to do so, especially without a comments.
>
> For sync mode, only RX is enabled, the register of tx should be set, so call the
> Set_dai_sysclk again.
Yea, I understood that. But why not just replace RX with TX on the
register-writing level? Do we need to set both TCCR and RCCR? Your
change in hw_params() only sets TCCR inside fsl_esai_set_bclk(), so
we probably only need to change TCCR for recordings running in sync
mode, right?
>From the commit message, it feels like that only the clock-related
fields in the TX registers need to be set. Things like calculation
and setting the direction of HCKx pin don't need to run again.
> > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > snd_pcm_substream *substream,
> > >
> > > bclk = params_rate(params) * slot_width * esai_priv->slots;
> > >
> > > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > +bclk);
> > > if (ret)
> > > return ret;
> > >
> > > + if (esai_priv->synchronous && !tx) {
> > > + /* Use Normal mode to support monaural audio */
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > + ESAI_xCR_xMOD_MASK,
> > params_channels(params) > 1 ?
> > > + ESAI_xCR_xMOD_NETWORK : 0);
> > > +
> > > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > mask, val);
> > > + }
> >
> > Does synchronous mode require to set both TCR and RCR? or just TCR?
> Both TCR and RCR. RCR will be set in normal flow.
OK. Settings both xCRs makes sense. Would you please try this:
===============
@@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ /* Synchronous mode uses TX clock generator */
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx, bclk);
if (ret)
return ret;
+ mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
+ val = ESAI_xCR_xSWS(slot_width, width);
/* Use Normal mode to support monaural audio */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
- ESAI_xCR_xMOD_NETWORK : 0);
+ val |= params_channels(params) > 1 ? ESAI_xCR_xMOD_NETWORK : 0;
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Recording in synchronous mode needs to set TCR also */
+ if (!tx && esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
@@ -556,10 +577,10 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
- mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
- val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Only TCR has padding bit and needs to be set for synchronous mode */
+ if (tx || esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_PADC, ESAI_xCR_PADC);
/* Remove ESAI personal reset by configuring ESAI_PCRC and ESAI_PRRC */
regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
===============
It merges those plain settings so we won't need to change both
sides if there's a modification of xCR settings in the future.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: fsl_esai: Support synchronous mode
@ 2019-04-02 20:01 ` Nicolin Chen
0 siblings, 0 replies; 8+ messages in thread
From: Nicolin Chen @ 2019-04-02 20:01 UTC (permalink / raw)
To: S.j. Wang; +Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
> > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > In ESAI synchronous mode, the clock is generated by Tx, So we should
> > > always set registers of Tx which relate with the bit clock and frame
> > > clock generation (TCCR, TCR, ECR), even there is only Rx is working.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index
> > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > --- a/sound/soc/fsl/fsl_esai.c
> > > +++ b/sound/soc/fsl/fsl_esai.c
> > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > snd_soc_dai *dai, int clk_id,
> > > return -EINVAL;
> > > }
> > >
> > > + if (esai_priv->synchronous && !tx) {
> > > + switch (clk_id) {
> > > + case ESAI_HCKR_FSYS:
> > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > + freq, dir);
> > > + break;
> > > + case ESAI_HCKR_EXTAL:
> > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > + freq, dir);
> >
> > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It feels very
> > confusing to do so, especially without a comments.
>
> For sync mode, only RX is enabled, the register of tx should be set, so call the
> Set_dai_sysclk again.
Yea, I understood that. But why not just replace RX with TX on the
register-writing level? Do we need to set both TCCR and RCCR? Your
change in hw_params() only sets TCCR inside fsl_esai_set_bclk(), so
we probably only need to change TCCR for recordings running in sync
mode, right?
From the commit message, it feels like that only the clock-related
fields in the TX registers need to be set. Things like calculation
and setting the direction of HCKx pin don't need to run again.
> > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > snd_pcm_substream *substream,
> > >
> > > bclk = params_rate(params) * slot_width * esai_priv->slots;
> > >
> > > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > +bclk);
> > > if (ret)
> > > return ret;
> > >
> > > + if (esai_priv->synchronous && !tx) {
> > > + /* Use Normal mode to support monaural audio */
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > + ESAI_xCR_xMOD_MASK,
> > params_channels(params) > 1 ?
> > > + ESAI_xCR_xMOD_NETWORK : 0);
> > > +
> > > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > mask, val);
> > > + }
> >
> > Does synchronous mode require to set both TCR and RCR? or just TCR?
> Both TCR and RCR. RCR will be set in normal flow.
OK. Settings both xCRs makes sense. Would you please try this:
===============
@@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
bclk = params_rate(params) * slot_width * esai_priv->slots;
- ret = fsl_esai_set_bclk(dai, tx, bclk);
+ /* Synchronous mode uses TX clock generator */
+ ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx, bclk);
if (ret)
return ret;
+ mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
+ val = ESAI_xCR_xSWS(slot_width, width);
/* Use Normal mode to support monaural audio */
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
- ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
- ESAI_xCR_xMOD_NETWORK : 0);
+ val |= params_channels(params) > 1 ? ESAI_xCR_xMOD_NETWORK : 0;
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Recording in synchronous mode needs to set TCR also */
+ if (!tx && esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR, mask, val);
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR);
@@ -556,10 +577,10 @@ static int fsl_esai_hw_params(struct snd_pcm_substream *substream,
regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
- mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
- val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
-
- regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
+ /* Only TCR has padding bit and needs to be set for synchronous mode */
+ if (tx || esai_priv->synchronous)
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_PADC, ESAI_xCR_PADC);
/* Remove ESAI personal reset by configuring ESAI_PCRC and ESAI_PRRC */
regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
===============
It merges those plain settings so we won't need to change both
sides if there's a modification of xCR settings in the future.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: fsl_esai: Support synchronous mode
2019-04-02 20:01 ` Nicolin Chen
@ 2019-04-03 10:03 ` S.j. Wang
-1 siblings, 0 replies; 8+ messages in thread
From: S.j. Wang @ 2019-04-03 10:03 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
Hi
>
> > > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > > In ESAI synchronous mode, the clock is generated by Tx, So we
> > > > should always set registers of Tx which relate with the bit clock
> > > > and frame clock generation (TCCR, TCR, ECR), even there is only Rx is
> working.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> > > > index
> > > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > > --- a/sound/soc/fsl/fsl_esai.c
> > > > +++ b/sound/soc/fsl/fsl_esai.c
> > > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > > snd_soc_dai *dai, int clk_id,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + switch (clk_id) {
> > > > + case ESAI_HCKR_FSYS:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > > + freq, dir);
> > > > + break;
> > > > + case ESAI_HCKR_EXTAL:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > > + freq, dir);
> > >
> > > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It
> > > feels very confusing to do so, especially without a comments.
> >
> > For sync mode, only RX is enabled, the register of tx should be set,
> > so call the Set_dai_sysclk again.
>
> Yea, I understood that. But why not just replace RX with TX on the register-
> writing level? Do we need to set both TCCR and RCCR? Your change in
> hw_params() only sets TCCR inside fsl_esai_set_bclk(), so we probably only
> need to change TCCR for recordings running in sync mode, right?
>
> From the commit message, it feels like that only the clock-related fields in
> the TX registers need to be set. Things like calculation and setting the
> direction of HCKx pin don't need to run again.
>
> > > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > > snd_pcm_substream *substream,
> > > >
> > > > bclk = params_rate(params) * slot_width * esai_priv->slots;
> > > >
> > > > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > > +bclk);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + /* Use Normal mode to support monaural audio */
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > > + ESAI_xCR_xMOD_MASK,
> > > params_channels(params) > 1 ?
> > > > + ESAI_xCR_xMOD_NETWORK : 0);
> > > > +
> > > > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > mask, val);
> > > > + }
> > >
> > > Does synchronous mode require to set both TCR and RCR? or just TCR?
>
> > Both TCR and RCR. RCR will be set in normal flow.
>
> OK. Settings both xCRs makes sense. Would you please try this:
>
> ===============
> @@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct
> snd_pcm_substream *substream,
>
> bclk = params_rate(params) * slot_width * esai_priv->slots;
>
> - ret = fsl_esai_set_bclk(dai, tx, bclk);
> + /* Synchronous mode uses TX clock generator */
> + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx,
> + bclk);
> if (ret)
> return ret;
>
> + mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
> + val = ESAI_xCR_xSWS(slot_width, width);
> /* Use Normal mode to support monaural audio */
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> - ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
> - ESAI_xCR_xMOD_NETWORK : 0);
> + val |= params_channels(params) > 1 ? ESAI_xCR_xMOD_NETWORK : 0;
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> + /* Recording in synchronous mode needs to set TCR also */
> + if (!tx && esai_priv->synchronous)
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + mask, val);
>
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
> ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR); @@ -556,10
> +577,10 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
> *substream,
>
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
>
> - mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
> - val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
> -
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> + /* Only TCR has padding bit and needs to be set for synchronous
> mode */
> + if (tx || esai_priv->synchronous)
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_PADC, ESAI_xCR_PADC);
>
> /* Remove ESAI personal reset by configuring ESAI_PCRC and
> ESAI_PRRC */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> ===============
>
> It merges those plain settings so we won't need to change both sides if
> there's a modification of xCR settings in the future.
Thanks for reviewing, will send v2. And the ESAI_xCR_PADC, ESAI_xCR_xMOD_NETWORK
Is not needed in sync mode.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ASoC: fsl_esai: Support synchronous mode
@ 2019-04-03 10:03 ` S.j. Wang
0 siblings, 0 replies; 8+ messages in thread
From: S.j. Wang @ 2019-04-03 10:03 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, timur, Xiubo.Lee, festevam, broonie, linuxppc-dev
Hi
>
> > > On Mon, Apr 01, 2019 at 11:39:10AM +0000, S.j. Wang wrote:
> > > > In ESAI synchronous mode, the clock is generated by Tx, So we
> > > > should always set registers of Tx which relate with the bit clock
> > > > and frame clock generation (TCCR, TCR, ECR), even there is only Rx is
> working.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > sound/soc/fsl/fsl_esai.c | 28 +++++++++++++++++++++++++++-
> > > > 1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> > > > index
> > > > 3623aa9a6f2e..d9fcddd55c02 100644
> > > > --- a/sound/soc/fsl/fsl_esai.c
> > > > +++ b/sound/soc/fsl/fsl_esai.c
> > > > @@ -230,6 +230,21 @@ static int fsl_esai_set_dai_sysclk(struct
> > > snd_soc_dai *dai, int clk_id,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + switch (clk_id) {
> > > > + case ESAI_HCKR_FSYS:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_FSYS,
> > > > + freq, dir);
> > > > + break;
> > > > + case ESAI_HCKR_EXTAL:
> > > > + fsl_esai_set_dai_sysclk(dai, ESAI_HCKT_EXTAL,
> > > > + freq, dir);
> > >
> > > Not sure why you call set_dai_sysclk inside set_dai_sysclk again. It
> > > feels very confusing to do so, especially without a comments.
> >
> > For sync mode, only RX is enabled, the register of tx should be set,
> > so call the Set_dai_sysclk again.
>
> Yea, I understood that. But why not just replace RX with TX on the register-
> writing level? Do we need to set both TCCR and RCCR? Your change in
> hw_params() only sets TCCR inside fsl_esai_set_bclk(), so we probably only
> need to change TCCR for recordings running in sync mode, right?
>
> From the commit message, it feels like that only the clock-related fields in
> the TX registers need to be set. Things like calculation and setting the
> direction of HCKx pin don't need to run again.
>
> > > > @@ -537,10 +552,21 @@ static int fsl_esai_hw_params(struct
> > > > snd_pcm_substream *substream,
> > > >
> > > > bclk = params_rate(params) * slot_width * esai_priv->slots;
> > > >
> > > > - ret = fsl_esai_set_bclk(dai, tx, bclk);
> > > > + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous ? true : tx,
> > > > +bclk);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + if (esai_priv->synchronous && !tx) {
> > > > + /* Use Normal mode to support monaural audio */
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > > + ESAI_xCR_xMOD_MASK,
> > > params_channels(params) > 1 ?
> > > > + ESAI_xCR_xMOD_NETWORK : 0);
> > > > +
> > > > + mask = ESAI_xCR_xSWS_MASK | ESAI_xCR_PADC;
> > > > + val = ESAI_xCR_xSWS(slot_width, width) | ESAI_xCR_PADC;
> > > > + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> > > mask, val);
> > > > + }
> > >
> > > Does synchronous mode require to set both TCR and RCR? or just TCR?
>
> > Both TCR and RCR. RCR will be set in normal flow.
>
> OK. Settings both xCRs makes sense. Would you please try this:
>
> ===============
> @@ -537,14 +552,20 @@ static int fsl_esai_hw_params(struct
> snd_pcm_substream *substream,
>
> bclk = params_rate(params) * slot_width * esai_priv->slots;
>
> - ret = fsl_esai_set_bclk(dai, tx, bclk);
> + /* Synchronous mode uses TX clock generator */
> + ret = fsl_esai_set_bclk(dai, esai_priv->synchronous || tx,
> + bclk);
> if (ret)
> return ret;
>
> + mask = ESAI_xCR_xMOD_MASK | ESAI_xCR_xSWS_MASK;
> + val = ESAI_xCR_xSWS(slot_width, width);
> /* Use Normal mode to support monaural audio */
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx),
> - ESAI_xCR_xMOD_MASK, params_channels(params) > 1 ?
> - ESAI_xCR_xMOD_NETWORK : 0);
> + val |= params_channels(params) > 1 ? ESAI_xCR_xMOD_NETWORK : 0;
> +
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> + /* Recording in synchronous mode needs to set TCR also */
> + if (!tx && esai_priv->synchronous)
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + mask, val);
>
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx),
> ESAI_xFCR_xFR_MASK, ESAI_xFCR_xFR); @@ -556,10
> +577,10 @@ static int fsl_esai_hw_params(struct snd_pcm_substream
> *substream,
>
> regmap_update_bits(esai_priv->regmap, REG_ESAI_xFCR(tx), mask, val);
>
> - mask = ESAI_xCR_xSWS_MASK | (tx ? ESAI_xCR_PADC : 0);
> - val = ESAI_xCR_xSWS(slot_width, width) | (tx ? ESAI_xCR_PADC : 0);
> -
> - regmap_update_bits(esai_priv->regmap, REG_ESAI_xCR(tx), mask, val);
> + /* Only TCR has padding bit and needs to be set for synchronous
> mode */
> + if (tx || esai_priv->synchronous)
> + regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
> + ESAI_xCR_PADC, ESAI_xCR_PADC);
>
> /* Remove ESAI personal reset by configuring ESAI_PCRC and
> ESAI_PRRC */
> regmap_update_bits(esai_priv->regmap, REG_ESAI_PRRC,
> ===============
>
> It merges those plain settings so we won't need to change both sides if
> there's a modification of xCR settings in the future.
Thanks for reviewing, will send v2. And the ESAI_xCR_PADC, ESAI_xCR_xMOD_NETWORK
Is not needed in sync mode.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-03 10:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-01 11:39 [PATCH] ASoC: fsl_esai: Support synchronous mode S.j. Wang
2019-04-01 18:17 ` Nicolin Chen
2019-04-02 10:16 ` S.j. Wang
2019-04-02 10:16 ` S.j. Wang
2019-04-02 20:01 ` Nicolin Chen
2019-04-02 20:01 ` Nicolin Chen
2019-04-03 10:03 ` S.j. Wang
2019-04-03 10:03 ` S.j. Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.