* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 8:47 ` Andreas Henriksson
0 siblings, 0 replies; 24+ messages in thread
From: Andreas Henriksson @ 2023-07-06 8:47 UTC (permalink / raw)
To: Fabio Estevam
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Shengjiu Wang, linux-kernel
Hello Shengjiu, Fabio,
On Thu, May 19, 2022 at 10:23:06AM -0300, Fabio Estevam wrote:
> Hi Shengjiu,
>
> On Thu, May 19, 2022 at 9:49 AM Shengjiu Wang <shengjiu.wang@nxp.com> wrote:
>
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index fa950dde5310..dae16a14f177 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -437,6 +437,12 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
> > FSL_SAI_CR2_DIV_MASK | FSL_SAI_CR2_BYP,
> > savediv / 2 - 1);
> >
> > + if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
>
> Isn't it a bit fragile to take this decision based on the number of
> SAI registers in the SoC?
>
> What about adding a specific field in soc_data for such a purpose?
We've been working on an i.MX8MP where MCLK needs to be input and found
that this enables the MCLK as output despite not having set the
`fsl,sai-mclk-direction-output;` devicetree property in our DT.
Reverting the patch fixes the issues for us.
I have to say that the code comment made me a bit confused, but once
I found the commit message I understood why this code existed.
If this is really i.MX8MM specific maybe mention that in the code
comment and please make the code actually only trigger on i.MX8MM.
It seems to me like these all fulfill the current criteria:
imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
Should I report this in bugzilla.kernel.org ?
Regards,
Andreas Henriksson
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 8:47 ` Andreas Henriksson
@ 2023-07-06 11:06 ` Shengjiu Wang
-1 siblings, 0 replies; 24+ messages in thread
From: Shengjiu Wang @ 2023-07-06 11:06 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Fabio Estevam, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
On Thu, Jul 6, 2023 at 4:47 PM Andreas Henriksson <andreas@fatal.se> wrote:
> Hello Shengjiu, Fabio,
>
> On Thu, May 19, 2022 at 10:23:06AM -0300, Fabio Estevam wrote:
> > Hi Shengjiu,
> >
> > On Thu, May 19, 2022 at 9:49 AM Shengjiu Wang <shengjiu.wang@nxp.com>
> wrote:
> >
> > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > index fa950dde5310..dae16a14f177 100644
> > > --- a/sound/soc/fsl/fsl_sai.c
> > > +++ b/sound/soc/fsl/fsl_sai.c
> > > @@ -437,6 +437,12 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
> *dai, bool tx, u32 freq)
> > > FSL_SAI_CR2_DIV_MASK |
> FSL_SAI_CR2_BYP,
> > > savediv / 2 - 1);
> > >
> > > + if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
> >
> > Isn't it a bit fragile to take this decision based on the number of
> > SAI registers in the SoC?
> >
> > What about adding a specific field in soc_data for such a purpose?
>
> We've been working on an i.MX8MP where MCLK needs to be input and found
> that this enables the MCLK as output despite not having set the
> `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> Reverting the patch fixes the issues for us.
>
> Good catch. seems there is an issue here.
best regards
wang shengjiu
I have to say that the code comment made me a bit confused, but once
> I found the commit message I understood why this code existed.
> If this is really i.MX8MM specific maybe mention that in the code
> comment and please make the code actually only trigger on i.MX8MM.
> It seems to me like these all fulfill the current criteria:
> imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
>
> Should I report this in bugzilla.kernel.org ?
>
> Regards,
> Andreas Henriksson
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 11:06 ` Shengjiu Wang
0 siblings, 0 replies; 24+ messages in thread
From: Shengjiu Wang @ 2023-07-06 11:06 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Fabio Estevam, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]
On Thu, Jul 6, 2023 at 4:47 PM Andreas Henriksson <andreas@fatal.se> wrote:
> Hello Shengjiu, Fabio,
>
> On Thu, May 19, 2022 at 10:23:06AM -0300, Fabio Estevam wrote:
> > Hi Shengjiu,
> >
> > On Thu, May 19, 2022 at 9:49 AM Shengjiu Wang <shengjiu.wang@nxp.com>
> wrote:
> >
> > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > index fa950dde5310..dae16a14f177 100644
> > > --- a/sound/soc/fsl/fsl_sai.c
> > > +++ b/sound/soc/fsl/fsl_sai.c
> > > @@ -437,6 +437,12 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
> *dai, bool tx, u32 freq)
> > > FSL_SAI_CR2_DIV_MASK |
> FSL_SAI_CR2_BYP,
> > > savediv / 2 - 1);
> > >
> > > + if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
> >
> > Isn't it a bit fragile to take this decision based on the number of
> > SAI registers in the SoC?
> >
> > What about adding a specific field in soc_data for such a purpose?
>
> We've been working on an i.MX8MP where MCLK needs to be input and found
> that this enables the MCLK as output despite not having set the
> `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> Reverting the patch fixes the issues for us.
>
> Good catch. seems there is an issue here.
best regards
wang shengjiu
I have to say that the code comment made me a bit confused, but once
> I found the commit message I understood why this code existed.
> If this is really i.MX8MM specific maybe mention that in the code
> comment and please make the code actually only trigger on i.MX8MM.
> It seems to me like these all fulfill the current criteria:
> imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
>
> Should I report this in bugzilla.kernel.org ?
>
> Regards,
> Andreas Henriksson
>
[-- Attachment #2: Type: text/html, Size: 2649 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 8:47 ` Andreas Henriksson
@ 2023-07-06 11:08 ` Fabio Estevam
-1 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 11:08 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Shengjiu Wang, Nicolin Chen, Xiubo Li, Shengjiu Wang,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
Hi Andreas,
On Thu, Jul 6, 2023 at 5:47 AM Andreas Henriksson <andreas@fatal.se> wrote:
> We've been working on an i.MX8MP where MCLK needs to be input and found
> that this enables the MCLK as output despite not having set the
> `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> Reverting the patch fixes the issues for us.
>
> I have to say that the code comment made me a bit confused, but once
> I found the commit message I understood why this code existed.
> If this is really i.MX8MM specific maybe mention that in the code
> comment and please make the code actually only trigger on i.MX8MM.
> It seems to me like these all fulfill the current criteria:
> imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
>
> Should I report this in bugzilla.kernel.org ?
Should we do a fix like this?
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -1453,7 +1453,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
/* Select MCLK direction */
if (sai->mclk_direction_output &&
- sai->soc_data->max_register >= FSL_SAI_MCTL) {
+ sai->soc_data->max_register >= FSL_SAI_MCTL &&
sai->mclk_direction_output) {
regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
FSL_SAI_MCTL_MCLK_EN, FSL_SAI_MCTL_MCLK_EN);
}
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 11:08 ` Fabio Estevam
0 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 11:08 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Shengjiu Wang, linux-kernel
Hi Andreas,
On Thu, Jul 6, 2023 at 5:47 AM Andreas Henriksson <andreas@fatal.se> wrote:
> We've been working on an i.MX8MP where MCLK needs to be input and found
> that this enables the MCLK as output despite not having set the
> `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> Reverting the patch fixes the issues for us.
>
> I have to say that the code comment made me a bit confused, but once
> I found the commit message I understood why this code existed.
> If this is really i.MX8MM specific maybe mention that in the code
> comment and please make the code actually only trigger on i.MX8MM.
> It seems to me like these all fulfill the current criteria:
> imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
>
> Should I report this in bugzilla.kernel.org ?
Should we do a fix like this?
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -1453,7 +1453,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
/* Select MCLK direction */
if (sai->mclk_direction_output &&
- sai->soc_data->max_register >= FSL_SAI_MCTL) {
+ sai->soc_data->max_register >= FSL_SAI_MCTL &&
sai->mclk_direction_output) {
regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
FSL_SAI_MCTL_MCLK_EN, FSL_SAI_MCTL_MCLK_EN);
}
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 11:08 ` Fabio Estevam
@ 2023-07-06 11:19 ` Shengjiu Wang
-1 siblings, 0 replies; 24+ messages in thread
From: Shengjiu Wang @ 2023-07-06 11:19 UTC (permalink / raw)
To: Fabio Estevam
Cc: Andreas Henriksson, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
On Thu, Jul 6, 2023 at 7:08 PM Fabio Estevam <festevam@gmail.com> wrote:
> Hi Andreas,
>
> On Thu, Jul 6, 2023 at 5:47 AM Andreas Henriksson <andreas@fatal.se>
> wrote:
>
> > We've been working on an i.MX8MP where MCLK needs to be input and found
> > that this enables the MCLK as output despite not having set the
> > `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> > Reverting the patch fixes the issues for us.
> >
> > I have to say that the code comment made me a bit confused, but once
> > I found the commit message I understood why this code existed.
> > If this is really i.MX8MM specific maybe mention that in the code
> > comment and please make the code actually only trigger on i.MX8MM.
> > It seems to me like these all fulfill the current criteria:
> > imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
> >
> > Should I report this in bugzilla.kernel.org ?
>
> Should we do a fix like this?
>
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -1453,7 +1453,7 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>
> /* Select MCLK direction */
> if (sai->mclk_direction_output &&
> - sai->soc_data->max_register >= FSL_SAI_MCTL) {
> + sai->soc_data->max_register >= FSL_SAI_MCTL &&
> sai->mclk_direction_output) {
> regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
> FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> }
>
No, this is the code in probe().
The code with the issue is in fsl_sai_set_bclk().
The clean way for fixing is to remove the code in fsl_sai_set_bclk()
and add "fsl,sai-mclk-direction-output;" property in dts for some
node.
best regards
wang shengjiu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 11:19 ` Shengjiu Wang
0 siblings, 0 replies; 24+ messages in thread
From: Shengjiu Wang @ 2023-07-06 11:19 UTC (permalink / raw)
To: Fabio Estevam
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Andreas Henriksson, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]
On Thu, Jul 6, 2023 at 7:08 PM Fabio Estevam <festevam@gmail.com> wrote:
> Hi Andreas,
>
> On Thu, Jul 6, 2023 at 5:47 AM Andreas Henriksson <andreas@fatal.se>
> wrote:
>
> > We've been working on an i.MX8MP where MCLK needs to be input and found
> > that this enables the MCLK as output despite not having set the
> > `fsl,sai-mclk-direction-output;` devicetree property in our DT.
> > Reverting the patch fixes the issues for us.
> >
> > I have to say that the code comment made me a bit confused, but once
> > I found the commit message I understood why this code existed.
> > If this is really i.MX8MM specific maybe mention that in the code
> > comment and please make the code actually only trigger on i.MX8MM.
> > It seems to me like these all fulfill the current criteria:
> > imx7ulp, imx8mq, imx8mm, imx8mp, imx8ulp, imx93
> >
> > Should I report this in bugzilla.kernel.org ?
>
> Should we do a fix like this?
>
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -1453,7 +1453,7 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>
> /* Select MCLK direction */
> if (sai->mclk_direction_output &&
> - sai->soc_data->max_register >= FSL_SAI_MCTL) {
> + sai->soc_data->max_register >= FSL_SAI_MCTL &&
> sai->mclk_direction_output) {
> regmap_update_bits(sai->regmap, FSL_SAI_MCTL,
> FSL_SAI_MCTL_MCLK_EN,
> FSL_SAI_MCTL_MCLK_EN);
> }
>
No, this is the code in probe().
The code with the issue is in fsl_sai_set_bclk().
The clean way for fixing is to remove the code in fsl_sai_set_bclk()
and add "fsl,sai-mclk-direction-output;" property in dts for some
node.
best regards
wang shengjiu
[-- Attachment #2: Type: text/html, Size: 2896 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 11:19 ` Shengjiu Wang
@ 2023-07-06 11:32 ` Fabio Estevam
-1 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 11:32 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Andreas Henriksson, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> No, this is the code in probe().
> The code with the issue is in fsl_sai_set_bclk().
Yes, I put it in the wrong place.
> The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> and add "fsl,sai-mclk-direction-output;" property in dts for some
> node.
Yes, what about this?
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -507,7 +507,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
*dai, bool tx, u32 freq)
savediv / 2 - 1);
}
- if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
+ if (sai->soc_data->max_register >= FSL_SAI_MCTL &&
sai->mclk_direction_output) {
/* 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);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 11:32 ` Fabio Estevam
0 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 11:32 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Andreas Henriksson, linux-kernel
On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> No, this is the code in probe().
> The code with the issue is in fsl_sai_set_bclk().
Yes, I put it in the wrong place.
> The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> and add "fsl,sai-mclk-direction-output;" property in dts for some
> node.
Yes, what about this?
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -507,7 +507,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
*dai, bool tx, u32 freq)
savediv / 2 - 1);
}
- if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
+ if (sai->soc_data->max_register >= FSL_SAI_MCTL &&
sai->mclk_direction_output) {
/* 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);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 11:32 ` Fabio Estevam
@ 2023-07-06 12:34 ` Andreas Henriksson
-1 siblings, 0 replies; 24+ messages in thread
From: Andreas Henriksson @ 2023-07-06 12:34 UTC (permalink / raw)
To: Fabio Estevam
Cc: Shengjiu Wang, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
Hello Fabio,
Maybe I shouldn't comment as I'm already on deep water, but...
On Thu, Jul 06, 2023 at 08:32:57AM -0300, Fabio Estevam wrote:
> On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> > No, this is the code in probe().
> > The code with the issue is in fsl_sai_set_bclk().
>
> Yes, I put it in the wrong place.
>
> > The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> > and add "fsl,sai-mclk-direction-output;" property in dts for some
> > node.
>
> Yes, what about this?
>
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -507,7 +507,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
> *dai, bool tx, u32 freq)
> savediv / 2 - 1);
> }
>
> - if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
> + if (sai->soc_data->max_register >= FSL_SAI_MCTL &&
> sai->mclk_direction_output) {
> /* 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);
This is exactly the same as and thus redundant to the code already in
the probe function?!
If I understood it correctly, the problem Shengjiu was trying to adress
was that apparently on i.MX8MM (and only imx8mm?!), even when using MCLK
in *input*, you still need to enable it because otherwise BCLK will not
be generated. (I personally don't know anything about this or the imx8mm
variant though.)
The problem could probably equally well be worked around by always setting the
fsl,sai-mclk-direction-output; devicetree property on imx8mm, even when
using MCLK in input..... But I'm just guessing here to be honest. This
is just as I understood what purpose the initial patch that was merged
had.
The current state affects more devices than imx8mm though, making MCLK
in input mode not possible.
I think your initial review comment was spot on Fabio. There probably
needs to be a(n imx8mm) specific flag that says when this workaround
should be applied and gate the code in bclk function on that.
Atleast that's the only thing I can think of if my interpretation of the
problem for imx8mm is correct.
Regards,
Andreas Henriksson
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 12:34 ` Andreas Henriksson
0 siblings, 0 replies; 24+ messages in thread
From: Andreas Henriksson @ 2023-07-06 12:34 UTC (permalink / raw)
To: Fabio Estevam
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Shengjiu Wang, linux-kernel
Hello Fabio,
Maybe I shouldn't comment as I'm already on deep water, but...
On Thu, Jul 06, 2023 at 08:32:57AM -0300, Fabio Estevam wrote:
> On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> > No, this is the code in probe().
> > The code with the issue is in fsl_sai_set_bclk().
>
> Yes, I put it in the wrong place.
>
> > The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> > and add "fsl,sai-mclk-direction-output;" property in dts for some
> > node.
>
> Yes, what about this?
>
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -507,7 +507,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai
> *dai, bool tx, u32 freq)
> savediv / 2 - 1);
> }
>
> - if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
> + if (sai->soc_data->max_register >= FSL_SAI_MCTL &&
> sai->mclk_direction_output) {
> /* 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);
This is exactly the same as and thus redundant to the code already in
the probe function?!
If I understood it correctly, the problem Shengjiu was trying to adress
was that apparently on i.MX8MM (and only imx8mm?!), even when using MCLK
in *input*, you still need to enable it because otherwise BCLK will not
be generated. (I personally don't know anything about this or the imx8mm
variant though.)
The problem could probably equally well be worked around by always setting the
fsl,sai-mclk-direction-output; devicetree property on imx8mm, even when
using MCLK in input..... But I'm just guessing here to be honest. This
is just as I understood what purpose the initial patch that was merged
had.
The current state affects more devices than imx8mm though, making MCLK
in input mode not possible.
I think your initial review comment was spot on Fabio. There probably
needs to be a(n imx8mm) specific flag that says when this workaround
should be applied and gate the code in bclk function on that.
Atleast that's the only thing I can think of if my interpretation of the
problem for imx8mm is correct.
Regards,
Andreas Henriksson
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 12:34 ` Andreas Henriksson
@ 2023-07-06 13:22 ` Fabio Estevam
-1 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 13:22 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Shengjiu Wang, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
Hi Andreas,
On Thu, Jul 6, 2023 at 9:34 AM Andreas Henriksson <andreas@fatal.se> wrote:
> I think your initial review comment was spot on Fabio. There probably
> needs to be a(n imx8mm) specific flag that says when this workaround
> should be applied and gate the code in bclk function on that.
> Atleast that's the only thing I can think of if my interpretation of the
> problem for imx8mm is correct.
Yes, deciding whether MCLK_EN should act as a clock gate based on
the number of SAI registers seems fragile to me.
I have sent a proposal as RFC. Please give it a try if you have a chance.
I would like Shengjiu to confirm if imx8mn and imx93 should also
behave like imx8mm in this aspect.
Cheers
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 13:22 ` Fabio Estevam
0 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 13:22 UTC (permalink / raw)
To: Andreas Henriksson
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Shengjiu Wang, linux-kernel
Hi Andreas,
On Thu, Jul 6, 2023 at 9:34 AM Andreas Henriksson <andreas@fatal.se> wrote:
> I think your initial review comment was spot on Fabio. There probably
> needs to be a(n imx8mm) specific flag that says when this workaround
> should be applied and gate the code in bclk function on that.
> Atleast that's the only thing I can think of if my interpretation of the
> problem for imx8mm is correct.
Yes, deciding whether MCLK_EN should act as a clock gate based on
the number of SAI registers seems fragile to me.
I have sent a proposal as RFC. Please give it a try if you have a chance.
I would like Shengjiu to confirm if imx8mn and imx93 should also
behave like imx8mm in this aspect.
Cheers
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
2023-07-06 11:19 ` Shengjiu Wang
@ 2023-07-06 22:00 ` Fabio Estevam
-1 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 22:00 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Andreas Henriksson, Shengjiu Wang, Nicolin Chen, Xiubo Li,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Linux-ALSA, linuxppc-dev, linux-kernel, Hans Söderlund
On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> and add "fsl,sai-mclk-direction-output;" property in dts for some
> node.
Yes, after thinking more about it, I agree.
So what you are proposing is basically a revert of the patch in
Subject and I assume that the
reason you did the original patch was that you missed passing
fsl,sai-mclk-direction-output in DT.
I will send the revert.
Thanks
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ASoC: fsl_sai: Enable MCTL_MCLK_EN bit for master mode
@ 2023-07-06 22:00 ` Fabio Estevam
0 siblings, 0 replies; 24+ messages in thread
From: Fabio Estevam @ 2023-07-06 22:00 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Linux-ALSA, Xiubo Li, linuxppc-dev, Shengjiu Wang, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
Hans Söderlund, Andreas Henriksson, linux-kernel
On Thu, Jul 6, 2023 at 8:19 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> The clean way for fixing is to remove the code in fsl_sai_set_bclk()
> and add "fsl,sai-mclk-direction-output;" property in dts for some
> node.
Yes, after thinking more about it, I agree.
So what you are proposing is basically a revert of the patch in
Subject and I assume that the
reason you did the original patch was that you missed passing
fsl,sai-mclk-direction-output in DT.
I will send the revert.
Thanks
^ permalink raw reply [flat|nested] 24+ messages in thread