[PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Marek Vasut]

For SAI to generate MCLK on external SoC pad, the transmitter must be
enabled as well. With transmitter disabled, no clock are generated.
Enable the transmitter using the TERE bit.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: Shengjiu Wang <shengjiu.wang@gmail.com>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Xiubo Li <Xiubo.Lee@gmail.com>
To: alsa-devel@alsa-project.org
---
 sound/soc/fsl/fsl_sai.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 1c9be8a5dcb13..98c62027e5799 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
 		/* SAI is in master mode at this point, so enable MCLK */
 		regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
 				   FSL_SAI_MCTL_MCLK_EN, FSL_SAI_MCTL_MCLK_EN);
+
+		/* Transmitter must be enabled to generate MCLK on pad */
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
+				   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
 	}
 
 	return 0;
@@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device *pdev)
 	    sai->soc_data->max_register >= FSL_SAI_MCTL) {
 		regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
 				   FSL_SAI_MCTL_MCLK_EN, FSL_SAI_MCTL_MCLK_EN);
+
+		/* Transmitter must be enabled to generate MCLK on pad */
+		regmap_update_bits(sai->regmap,
+				   FSL_SAI_xCSR(1, sai->soc_data->reg_offset),
+				   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
 	}
 
 	ret = pm_runtime_put_sync(dev);
-- 
2.35.1

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Shengjiu Wang]

On Tue, Nov 8, 2022 at 5:52 AM Marek Vasut <marex@denx.de> wrote:

> For SAI to generate MCLK on external SoC pad, the transmitter must be
> enabled as well. With transmitter disabled, no clock are generated.
> Enable the transmitter using the TERE bit.
>

It is already done in trigger(), why need to do this operation in probe()
and set_bclk() again?

best regards
wang shengjiu

>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Nicolin Chen <nicoleotsuka@gmail.com>
> Cc: Shengjiu Wang <shengjiu.wang@gmail.com>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Xiubo Li <Xiubo.Lee@gmail.com>
> To: alsa-devel@alsa-project.org
> ---
>  sound/soc/fsl/fsl_sai.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 1c9be8a5dcb13..98c62027e5799 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai,
> bool tx, u32 freq)
>                 /* SAI is in master mode at this point, so enable MCLK */
>                 regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>                                    FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> +
> +               /* Transmitter must be enabled to generate MCLK on pad */
> +               regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>         }
>
>         return 0;
> @@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>             sai->soc_data->max_register >= FSL_SAI_MCTL) {
>                 regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>                                    FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> +
> +               /* Transmitter must be enabled to generate MCLK on pad */
> +               regmap_update_bits(sai->regmap,
> +                                  FSL_SAI_xCSR(1,
> sai->soc_data->reg_offset),
> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>         }
>
>         ret = pm_runtime_put_sync(dev);
> --
> 2.35.1
>
>

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Marek Vasut]

On 11/8/22 06:29, Shengjiu Wang wrote:
> On Tue, Nov 8, 2022 at 5:52 AM Marek Vasut <marex@denx.de> wrote:
> 
>> For SAI to generate MCLK on external SoC pad, the transmitter must be
>> enabled as well. With transmitter disabled, no clock are generated.
>> Enable the transmitter using the TERE bit.
>>
> 
> It is already done in trigger(), why need to do this operation in probe()
> and set_bclk() again?

The system I have here has SAI MCLK (output) connected to SGTL5000 codec 
MCLK (input) . If the SAI MCLK (output), the codec I2C interface won't 
work. That's why the MCLK must be enabled so early.

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Shengjiu Wang]

On Tue, Nov 8, 2022 at 5:52 AM Marek Vasut <marex@denx.de> wrote:

> For SAI to generate MCLK on external SoC pad, the transmitter must be
> enabled as well. With transmitter disabled, no clock are generated.
> Enable the transmitter using the TERE bit.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Nicolin Chen <nicoleotsuka@gmail.com>
> Cc: Shengjiu Wang <shengjiu.wang@gmail.com>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Xiubo Li <Xiubo.Lee@gmail.com>
> To: alsa-devel@alsa-project.org
> ---
>  sound/soc/fsl/fsl_sai.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 1c9be8a5dcb13..98c62027e5799 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai,
> bool tx, u32 freq)
>                 /* SAI is in master mode at this point, so enable MCLK */
>                 regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>                                    FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> +
> +               /* Transmitter must be enabled to generate MCLK on pad */
> +               regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>

No need to fix the transmitter,   FSL_SAI_xCSR(tx, ofs) should work also.



>         }
>
>         return 0;
> @@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>             sai->soc_data->max_register >= FSL_SAI_MCTL) {
>                 regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>                                    FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> +
> +               /* Transmitter must be enabled to generate MCLK on pad */
> +               regmap_update_bits(sai->regmap,
> +                                  FSL_SAI_xCSR(1,
> sai->soc_data->reg_offset),
> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>

After one time playback or recording,  the TERE is disabled, so this changes
in probe() only for the first time.  There is the same issue for the second
time.

best regards
wang shengjiu

>         }
>
>         ret = pm_runtime_put_sync(dev);
> --
> 2.35.1
>
>

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Marek Vasut]

On 11/9/22 10:30, Shengjiu Wang wrote:

[...]

>> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
>> index 1c9be8a5dcb13..98c62027e5799 100644
>> --- a/sound/soc/fsl/fsl_sai.c
>> +++ b/sound/soc/fsl/fsl_sai.c
>> @@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai,
>> bool tx, u32 freq)
>>                  /* SAI is in master mode at this point, so enable MCLK */
>>                  regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>>                                     FSL_SAI_MCTL_MCLK_EN,
>> FSL_SAI_MCTL_MCLK_EN);
>> +
>> +               /* Transmitter must be enabled to generate MCLK on pad */
>> +               regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
>> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>>
> 
> No need to fix the transmitter,   FSL_SAI_xCSR(tx, ofs) should work also.

That's probably not a good idea, since this could also enable the 
RE(receiver) part . We always need to enable the TE(transmitter) to 
generate MCLK.

>>          }
>>
>>          return 0;
>> @@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device
>> *pdev)
>>              sai->soc_data->max_register >= FSL_SAI_MCTL) {
>>                  regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>>                                     FSL_SAI_MCTL_MCLK_EN,
>> FSL_SAI_MCTL_MCLK_EN);
>> +
>> +               /* Transmitter must be enabled to generate MCLK on pad */
>> +               regmap_update_bits(sai->regmap,
>> +                                  FSL_SAI_xCSR(1,
>> sai->soc_data->reg_offset),
>> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>>
> 
> After one time playback or recording,  the TERE is disabled, so this changes
> in probe() only for the first time.  There is the same issue for the second
> time.

So what would you suggest to keep the MCLK clock generated always ?

I was almost tempted to turn the SAI into a clock provider, so that the 
codec could become its clock consumer and enable the MCLK on demand, but 
that seems to be a rather invasive change.

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Shengjiu Wang]

On Wed, Nov 9, 2022 at 8:55 PM Marek Vasut <marex@denx.de> wrote:

> On 11/9/22 10:30, Shengjiu Wang wrote:
>
> [...]
>
> >> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> >> index 1c9be8a5dcb13..98c62027e5799 100644
> >> --- a/sound/soc/fsl/fsl_sai.c
> >> +++ b/sound/soc/fsl/fsl_sai.c
> >> @@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
> *dai,
> >> bool tx, u32 freq)
> >>                  /* SAI is in master mode at this point, so enable MCLK
> */
> >>                  regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
> >>                                     FSL_SAI_MCTL_MCLK_EN,
> >> FSL_SAI_MCTL_MCLK_EN);
> >> +
> >> +               /* Transmitter must be enabled to generate MCLK on pad
> */
> >> +               regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
> >> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> >>
> >
> > No need to fix the transmitter,   FSL_SAI_xCSR(tx, ofs) should work also.
>
> That's probably not a good idea, since this could also enable the
> RE(receiver) part . We always need to enable the TE(transmitter) to
> generate MCLK.
>

TCSR.TERE and RCSR.TERE all can enable the MCLK.
if use the FSL_SAI_xCSR(tx, ofs), both cases can be covered.
if the bclk is generated by TX, then enable the TE(transmitter) to
generate MCLK.  or if the bclk is generated by RX, then enable
the RE(receiver) to generate MCLK.


>
> >>          }
> >>
> >>          return 0;
> >> @@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device
> >> *pdev)
> >>              sai->soc_data->max_register >= FSL_SAI_MCTL) {
> >>                  regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
> >>                                     FSL_SAI_MCTL_MCLK_EN,
> >> FSL_SAI_MCTL_MCLK_EN);
> >> +
> >> +               /* Transmitter must be enabled to generate MCLK on pad
> */
> >> +               regmap_update_bits(sai->regmap,
> >> +                                  FSL_SAI_xCSR(1,
> >> sai->soc_data->reg_offset),
> >> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> >>
> >
> > After one time playback or recording,  the TERE is disabled, so this
> changes
> > in probe() only for the first time.  There is the same issue for the
> second
> > time.
>
> So what would you suggest to keep the MCLK clock generated always ?
>
> I was almost tempted to turn the SAI into a clock provider, so that the
> codec could become its clock consumer and enable the MCLK on demand, but
> that seems to be a rather invasive change.
>

May you can try to enhance the drivers/clk/clk-fsl-sai.c

best regards
wang shengjiu

Re: [PATCH] ASoC: fsl_sai: Enable transmitter when generating MCLK

[Marek Vasut]

On 11/14/22 08:55, Shengjiu Wang wrote:
> On Wed, Nov 9, 2022 at 8:55 PM Marek Vasut <marex@denx.de> wrote:
> 
>> On 11/9/22 10:30, Shengjiu Wang wrote:
>>
>> [...]
>>
>>>> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
>>>> index 1c9be8a5dcb13..98c62027e5799 100644
>>>> --- a/sound/soc/fsl/fsl_sai.c
>>>> +++ b/sound/soc/fsl/fsl_sai.c
>>>> @@ -503,6 +503,10 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
>> *dai,
>>>> bool tx, u32 freq)
>>>>                   /* SAI is in master mode at this point, so enable MCLK
>> */
>>>>                   regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>>>>                                      FSL_SAI_MCTL_MCLK_EN,
>>>> FSL_SAI_MCTL_MCLK_EN);
>>>> +
>>>> +               /* Transmitter must be enabled to generate MCLK on pad
>> */
>>>> +               regmap_update_bits(sai->regmap, FSL_SAI_xCSR(1, ofs),
>>>> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>>>>
>>>
>>> No need to fix the transmitter,   FSL_SAI_xCSR(tx, ofs) should work also.
>>
>> That's probably not a good idea, since this could also enable the
>> RE(receiver) part . We always need to enable the TE(transmitter) to
>> generate MCLK.
>>
> 
> TCSR.TERE and RCSR.TERE all can enable the MCLK.
> if use the FSL_SAI_xCSR(tx, ofs), both cases can be covered.
> if the bclk is generated by TX, then enable the TE(transmitter) to
> generate MCLK.  or if the bclk is generated by RX, then enable
> the RE(receiver) to generate MCLK.

I will need to do a full re-test to validate this claim.

>>>> @@ -1445,6 +1449,11 @@ static int fsl_sai_probe(struct platform_device
>>>> *pdev)
>>>>               sai->soc_data->max_register >= FSL_SAI_MCTL) {
>>>>                   regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
>>>>                                      FSL_SAI_MCTL_MCLK_EN,
>>>> FSL_SAI_MCTL_MCLK_EN);
>>>> +
>>>> +               /* Transmitter must be enabled to generate MCLK on pad
>> */
>>>> +               regmap_update_bits(sai->regmap,
>>>> +                                  FSL_SAI_xCSR(1,
>>>> sai->soc_data->reg_offset),
>>>> +                                  FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
>>>>
>>>
>>> After one time playback or recording,  the TERE is disabled, so this
>> changes
>>> in probe() only for the first time.  There is the same issue for the
>> second
>>> time.
>>
>> So what would you suggest to keep the MCLK clock generated always ?
>>
>> I was almost tempted to turn the SAI into a clock provider, so that the
>> codec could become its clock consumer and enable the MCLK on demand, but
>> that seems to be a rather invasive change.
>>
> 
> May you can try to enhance the drivers/clk/clk-fsl-sai.c

This is not really useful in this case, this lets users use the SAI as a 
plain clock source. I use the SAI as audio IP, all I need in addition to 
that is enable the MCLK as codec Master Clock to drive the codec 
register file and whatever internal state it has.