diff mbox series

[1/3] ASoC: simple-card-utils: Fix device module clock

Message ID 1612939421-19900-2-git-send-email-spujar@nvidia.com
State Accepted
Commit 1e30f642cf2939bbdac82ea0dd3071232670b5ab
Headers show
Series Use clocks property in a device node | expand

Commit Message

Sameer Pujar Feb. 10, 2021, 6:43 a.m. UTC
If "clocks = <&xxx>" is specified from the CPU or Codec component
device node, the clock is not getting enabled. Thus audio playback
or capture fails.

Fix this by populating "simple_dai->clk" field when clocks property
is specified from device node as well. Also tidy up by re-organising
conditional statements of parsing logic.

Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 sound/soc/generic/simple-card-utils.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Kuninori Morimoto Feb. 11, 2021, 11:44 p.m. UTC | #1
Hi Sameer

> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index bc0b62e..0754d70 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>  	 *  or device's module clock.
>  	 */
>  	clk = devm_get_clk_from_child(dev, node, NULL);
> -	if (!IS_ERR(clk)) {
> -		simple_dai->sysclk = clk_get_rate(clk);
> +	if (IS_ERR(clk))
> +		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>  
> +	if (!IS_ERR(clk)) {
>  		simple_dai->clk = clk;
> -	} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> +		simple_dai->sysclk = clk_get_rate(clk);
> +	} else if (!of_property_read_u32(node, "system-clock-frequency",
> +					 &val)) {
>  		simple_dai->sysclk = val;
> -	} else {
> -		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> -		if (!IS_ERR(clk))
> -			simple_dai->sysclk = clk_get_rate(clk);
>  	}

The comment is indicating that that the clock parsing order,
but this patch exchanges it.
This comment also should be updated, I think.

	/*
	 * Parse dai->sysclk come from "clocks = <&xxx>"
	 * (if system has common clock)
	 *  or "system-clock-frequency = <xxx>"
	 *  or device's module clock.
	 */

asoc_simple_set_clk_rate() will be called if it has simple_dai->clk.
CPU or Codec component clock rate will be exchanged by this patch, I think.
I'm not sure the effect of this patch to existing boards.

And also, this patch has too many unneeded exchange,
thus it was difficult to read for me.
I think it can be simply like this ?
It is understandable what it want to do.

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 8c423afb9d2e..d441890de4dc 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev,
 	 *  or device's module clock.
 	 */
 	clk = devm_get_clk_from_child(dev, node, NULL);
+	if (IS_ERR(clk))
+		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
+
 	if (!IS_ERR(clk)) {
 		simple_dai->sysclk = clk_get_rate(clk);
-
 		simple_dai->clk = clk;
 	} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
 		simple_dai->sysclk = val;
-	} else {
-		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
-		if (!IS_ERR(clk))
-			simple_dai->sysclk = clk_get_rate(clk);
 	}
 
 	if (of_property_read_bool(node, "system-clock-direction-out"))




Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar Feb. 14, 2021, 5:56 p.m. UTC | #2
Hi Morimoto-san,


On 2/12/2021 5:14 AM, Kuninori Morimoto wrote:
>> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
>> index bc0b62e..0754d70 100644
>> --- a/sound/soc/generic/simple-card-utils.c
>> +++ b/sound/soc/generic/simple-card-utils.c
>> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>>         *  or device's module clock.
>>         */
>>        clk = devm_get_clk_from_child(dev, node, NULL);
>> -     if (!IS_ERR(clk)) {
>> -             simple_dai->sysclk = clk_get_rate(clk);
>> +     if (IS_ERR(clk))
>> +             clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>>
>> +     if (!IS_ERR(clk)) {
>>                simple_dai->clk = clk;
>> -     } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>> +             simple_dai->sysclk = clk_get_rate(clk);
>> +     } else if (!of_property_read_u32(node, "system-clock-frequency",
>> +                                      &val)) {
>>                simple_dai->sysclk = val;
>> -     } else {
>> -             clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>> -             if (!IS_ERR(clk))
>> -                     simple_dai->sysclk = clk_get_rate(clk);
>>        }
> The comment is indicating that that the clock parsing order,
> but this patch exchanges it.
> This comment also should be updated, I think.
>
>          /*
>           * Parse dai->sysclk come from "clocks = <&xxx>"
>           * (if system has common clock)
>           *  or "system-clock-frequency = <xxx>"
>           *  or device's module clock.
>           */

Yes, this can be rephrased now.

> asoc_simple_set_clk_rate() will be called if it has simple_dai->clk.
> CPU or Codec component clock rate will be exchanged by this patch, I think.
> I'm not sure the effect of this patch to existing boards.

If CPU or Codec node does not specifiy "mclk-fs" factor, 
asoc_simple_set_clk_rate() won't be called. So I don't think there would 
be any effect w.r.t clock rate. With this patch clocks would get 
enabled/disabled.

>
> And also, this patch has too many unneeded exchange,
> thus it was difficult to read for me.
> I think it can be simply like this ?
> It is understandable what it want to do.

I think the patch does exactly the same thing as what you are suggesting 
below. Am I missing anything?

>
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index 8c423afb9d2e..d441890de4dc 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev,
>           *  or device's module clock.
>           */
>          clk = devm_get_clk_from_child(dev, node, NULL);
> +       if (IS_ERR(clk))
> +               clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> +
>          if (!IS_ERR(clk)) {
>                  simple_dai->sysclk = clk_get_rate(clk);
> -
>                  simple_dai->clk = clk;
>          } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>                  simple_dai->sysclk = val;
> -       } else {
> -               clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> -               if (!IS_ERR(clk))
> -                       simple_dai->sysclk = clk_get_rate(clk);
>          }
>
>          if (of_property_read_bool(node, "system-clock-direction-out"))
Kuninori Morimoto Feb. 14, 2021, 11:25 p.m. UTC | #3
Hi Sameer

> >          /*
> >           * Parse dai->sysclk come from "clocks = <&xxx>"
> >           * (if system has common clock)
> >           *  or "system-clock-frequency = <xxx>"
> >           *  or device's module clock.
> >           */
> 
> Yes, this can be rephrased now.

Thanks.
It is not a big-deal. no streass :)

> > And also, this patch has too many unneeded exchange,
> > thus it was difficult to read for me.
> > I think it can be simply like this ?
> > It is understandable what it want to do.
> 
> I think the patch does exactly the same thing as what you are
> suggesting below. Am I missing anything?

Yes, it is 100% same, but is simple patch.
I wanted to tell was it is easy to read/understand.
Your patch is already applied, so nothing we can do now ;)


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Michael Walle March 9, 2021, 2:41 p.m. UTC | #4
Hi,

> If "clocks = <&xxx>" is specified from the CPU or Codec component
> device node, the clock is not getting enabled. Thus audio playback
> or capture fails.
> 
> Fix this by populating "simple_dai->clk" field when clocks property
> is specified from device node as well. Also tidy up by re-organising
> conditional statements of parsing logic.
> 
> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>

This actually breaks sound on my board
(arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
The codec on this board (wm8904) has a fixed clock input (only distinct
frequencies are supported) and uses the FLL of the codec to generate the
desired sample rate.

It seems that after this patch the clock rate of the codecs clock (rather
than the FLL) is tried to be changed. Which fails, because it doesn't
support arbitrary frequencies.

-michael

> ---
>  sound/soc/generic/simple-card-utils.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index bc0b62e..0754d70 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>  	 *  or device's module clock.
>  	 */
>  	clk = devm_get_clk_from_child(dev, node, NULL);
> -	if (!IS_ERR(clk)) {
> -		simple_dai->sysclk = clk_get_rate(clk);
> +	if (IS_ERR(clk))
> +		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>  
> +	if (!IS_ERR(clk)) {
>  		simple_dai->clk = clk;
> -	} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> +		simple_dai->sysclk = clk_get_rate(clk);
> +	} else if (!of_property_read_u32(node, "system-clock-frequency",
> +					 &val)) {
>  		simple_dai->sysclk = val;
> -	} else {
> -		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> -		if (!IS_ERR(clk))
> -			simple_dai->sysclk = clk_get_rate(clk);
>  	}
>  
>  	if (of_property_read_bool(node, "system-clock-direction-out"))
> -- 
> 2.7.4
> 
>
Sameer Pujar March 9, 2021, 4:27 p.m. UTC | #5
Hi Michael,

On 3/9/2021 8:11 PM, Michael Walle wrote:
> External email: Use caution opening links or attachments
>
>
> Hi,
>
>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>> device node, the clock is not getting enabled. Thus audio playback
>> or capture fails.
>>
>> Fix this by populating "simple_dai->clk" field when clocks property
>> is specified from device node as well. Also tidy up by re-organising
>> conditional statements of parsing logic.
>>
>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> This actually breaks sound on my board
> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
> The codec on this board (wm8904) has a fixed clock input (only distinct
> frequencies are supported) and uses the FLL of the codec to generate the
> desired sample rate.
>
> It seems that after this patch the clock rate of the codecs clock (rather
> than the FLL) is tried to be changed. Which fails, because it doesn't
> support arbitrary frequencies.

Yes, after the given change the clock will be updated if "*mclk-fs" 
property is specified.

DT you mentioned has property "simple-audio-card,mclk-fs = <256>", which 
means you need a clock that is a function of sample rate. But as per 
above you want a fixed clock for MCLK. I think if you drop this 
property, the clock updates won't happen. Earlier for your case, this 
property was not used at all because the clock handle was not populated.

>
> -michael
>
>> ---
>>   sound/soc/generic/simple-card-utils.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
>> index bc0b62e..0754d70 100644
>> --- a/sound/soc/generic/simple-card-utils.c
>> +++ b/sound/soc/generic/simple-card-utils.c
>> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>>         *  or device's module clock.
>>         */
>>        clk = devm_get_clk_from_child(dev, node, NULL);
>> -     if (!IS_ERR(clk)) {
>> -             simple_dai->sysclk = clk_get_rate(clk);
>> +     if (IS_ERR(clk))
>> +             clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>>
>> +     if (!IS_ERR(clk)) {
>>                simple_dai->clk = clk;
>> -     } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>> +             simple_dai->sysclk = clk_get_rate(clk);
>> +     } else if (!of_property_read_u32(node, "system-clock-frequency",
>> +                                      &val)) {
>>                simple_dai->sysclk = val;
>> -     } else {
>> -             clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>> -             if (!IS_ERR(clk))
>> -                     simple_dai->sysclk = clk_get_rate(clk);
>>        }
>>
>>        if (of_property_read_bool(node, "system-clock-direction-out"))
>> --
>> 2.7.4
>>
>>
Michael Walle March 9, 2021, 10:30 p.m. UTC | #6
Hi Sameer,

Am 2021-03-09 17:27, schrieb Sameer Pujar:
> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>> device node, the clock is not getting enabled. Thus audio playback
>>> or capture fails.
>>> 
>>> Fix this by populating "simple_dai->clk" field when clocks property
>>> is specified from device node as well. Also tidy up by re-organising
>>> conditional statements of parsing logic.
>>> 
>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add 
>>> asoc_simple_card_parse_clk()")
>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> 
>> This actually breaks sound on my board
>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>> The codec on this board (wm8904) has a fixed clock input (only 
>> distinct
>> frequencies are supported) and uses the FLL of the codec to generate 
>> the
>> desired sample rate.
>> 
>> It seems that after this patch the clock rate of the codecs clock 
>> (rather
>> than the FLL) is tried to be changed. Which fails, because it doesn't
>> support arbitrary frequencies.
> 
> Yes, after the given change the clock will be updated if "*mclk-fs"
> property is specified.
> 
> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
> which means you need a clock that is a function of sample rate. But as
> per above you want a fixed clock for MCLK. I think if you drop this
> property, the clock updates won't happen. Earlier for your case, this
> property was not used at all because the clock handle was not
> populated.

You mean to drop the mclk-fs property? I can't do that because I
actually need a frequency of 256 * sample rate. But that doesn't
necessarily need to be the MCLK, because the codec itself has a
FLL/PLL which can be used to generate any frequency for a given
MCLK. So that is a valid scenario. See also commit 13409d27cb39
("ASoC: wm8904: configure sysclk/FLL automatically").

-michael
Sameer Pujar March 10, 2021, 2:50 p.m. UTC | #7
On 3/10/2021 4:00 AM, Michael Walle wrote:
> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>> device node, the clock is not getting enabled. Thus audio playback
>>>> or capture fails.
>>>>
>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>> is specified from device node as well. Also tidy up by re-organising
>>>> conditional statements of parsing logic.
>>>>
>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>> asoc_simple_card_parse_clk()")
>>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>
>>> This actually breaks sound on my board
>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>> The codec on this board (wm8904) has a fixed clock input (only
>>> distinct
>>> frequencies are supported) and uses the FLL of the codec to generate
>>> the
>>> desired sample rate.
>>>
>>> It seems that after this patch the clock rate of the codecs clock
>>> (rather
>>> than the FLL) is tried to be changed. Which fails, because it doesn't
>>> support arbitrary frequencies.
>>
>> Yes, after the given change the clock will be updated if "*mclk-fs"
>> property is specified.
>>
>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>> which means you need a clock that is a function of sample rate. But as
>> per above you want a fixed clock for MCLK. I think if you drop this
>> property, the clock updates won't happen. Earlier for your case, this
>> property was not used at all because the clock handle was not
>> populated.
>
> You mean to drop the mclk-fs property? I can't do that because I
> actually need a frequency of 256 * sample rate. But that doesn't
> necessarily need to be the MCLK, because the codec itself has a
> FLL/PLL which can be used to generate any frequency for a given
> MCLK. So that is a valid scenario. See also commit 13409d27cb39
> ("ASoC: wm8904: configure sysclk/FLL automatically").
>

If I read this correctly below is the configuration you need,
SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> sysclk

 From the doc simple-card.txt, "simple-audio-card,mclk-fs" is a scaling 
factor for MCLK and hence I am not sure if it is correct to have 
"*mclk-fs" property when MCLK is fixed. In simple cases, codec sysclk 
direclty depends on MCLK and set_sysclk() callback helps. Your case 
requires PLL configuration and set_pll() may be a better alternative. 
However simple-card does not offer this yet. But even if this is added, 
there should be a way to suggest PLL output requirement as a function of 
sample rate.
Michael Walle March 10, 2021, 6:14 p.m. UTC | #8
Hi Sameer, Hi Mark,

Am 2021-03-10 15:50, schrieb Sameer Pujar:
> On 3/10/2021 4:00 AM, Michael Walle wrote:
>> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>>> device node, the clock is not getting enabled. Thus audio playback
>>>>> or capture fails.
>>>>> 
>>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>>> is specified from device node as well. Also tidy up by 
>>>>> re-organising
>>>>> conditional statements of parsing logic.
>>>>> 
>>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>>> asoc_simple_card_parse_clk()")
>>>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>> 
>>>> This actually breaks sound on my board
>>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>>> The codec on this board (wm8904) has a fixed clock input (only
>>>> distinct
>>>> frequencies are supported) and uses the FLL of the codec to generate
>>>> the
>>>> desired sample rate.
>>>> 
>>>> It seems that after this patch the clock rate of the codecs clock
>>>> (rather
>>>> than the FLL) is tried to be changed. Which fails, because it 
>>>> doesn't
>>>> support arbitrary frequencies.
>>> 
>>> Yes, after the given change the clock will be updated if "*mclk-fs"
>>> property is specified.
>>> 
>>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>>> which means you need a clock that is a function of sample rate. But 
>>> as
>>> per above you want a fixed clock for MCLK. I think if you drop this
>>> property, the clock updates won't happen. Earlier for your case, this
>>> property was not used at all because the clock handle was not
>>> populated.
>> 
>> You mean to drop the mclk-fs property? I can't do that because I
>> actually need a frequency of 256 * sample rate. But that doesn't
>> necessarily need to be the MCLK, because the codec itself has a
>> FLL/PLL which can be used to generate any frequency for a given
>> MCLK. So that is a valid scenario. See also commit 13409d27cb39
>> ("ASoC: wm8904: configure sysclk/FLL automatically").
>> 
> 
> If I read this correctly below is the configuration you need,
> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> 
> sysclk
> 
> From the doc simple-card.txt, "simple-audio-card,mclk-fs" is a scaling
> factor for MCLK and hence I am not sure if it is correct to have
> "*mclk-fs" property when MCLK is fixed. In simple cases, codec sysclk
> direclty depends on MCLK and set_sysclk() callback helps. Your case
> requires PLL configuration and set_pll() may be a better alternative.
> However simple-card does not offer this yet. But even if this is
> added, there should be a way to suggest PLL output requirement as a
> function of sample rate.

But its also a scaling factor for the sysclk, then maybe the property
has a wrong name. And you might be right with your suggestion, but
as I said, this breaks sound on my board and whats even worse, it
breaks it for older kernel too, because of the Fixes tag.

Btw I'm pretty sure, the MCLK was enabled and disabled depending on
whether there was an audio stream, the last time I've measured the
clock.

Mark, can this patch please be reverted (with a Fixes tag) until
a proper fix is found which satisfies both needs?

-michael
Sameer Pujar March 10, 2021, 7:19 p.m. UTC | #9
On 3/10/2021 11:44 PM, Michael Walle wrote:

> Btw I'm pretty sure, the MCLK was enabled and disabled depending on
> whether there was an audio stream, the last time I've measured the
> clock.

This may be true in your case because wm8904 driver does an explicit 
clock enable/disable and does not rely on simple-card-utils for this. In 
my case it depends on simple-card-utils.
Michael Walle March 11, 2021, 10:27 a.m. UTC | #10
Hi Sameer,

Am 2021-03-10 15:50, schrieb Sameer Pujar:
> On 3/10/2021 4:00 AM, Michael Walle wrote:
>> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>>> device node, the clock is not getting enabled. Thus audio playback
>>>>> or capture fails.
>>>>> 
>>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>>> is specified from device node as well. Also tidy up by 
>>>>> re-organising
>>>>> conditional statements of parsing logic.
>>>>> 
>>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>>> asoc_simple_card_parse_clk()")
>>>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>> 
>>>> This actually breaks sound on my board
>>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>>> The codec on this board (wm8904) has a fixed clock input (only
>>>> distinct
>>>> frequencies are supported) and uses the FLL of the codec to generate
>>>> the
>>>> desired sample rate.
>>>> 
>>>> It seems that after this patch the clock rate of the codecs clock
>>>> (rather
>>>> than the FLL) is tried to be changed. Which fails, because it 
>>>> doesn't
>>>> support arbitrary frequencies.
>>> 
>>> Yes, after the given change the clock will be updated if "*mclk-fs"
>>> property is specified.
>>> 
>>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>>> which means you need a clock that is a function of sample rate. But 
>>> as
>>> per above you want a fixed clock for MCLK. I think if you drop this
>>> property, the clock updates won't happen. Earlier for your case, this
>>> property was not used at all because the clock handle was not
>>> populated.
>> 
>> You mean to drop the mclk-fs property? I can't do that because I
>> actually need a frequency of 256 * sample rate. But that doesn't
>> necessarily need to be the MCLK, because the codec itself has a
>> FLL/PLL which can be used to generate any frequency for a given
>> MCLK. So that is a valid scenario. See also commit 13409d27cb39
>> ("ASoC: wm8904: configure sysclk/FLL automatically").
>> 

I've had a closer look at this and it seems you're messing around
with the clock of the codec's node (which is _not_ a subnode of
the simple-audio-card). I don't think this is correct.

I guess you should rather set the clock property in the codec
subnode of the simple-audio-card, which is then picked up by the
simple-audio-card driver and changed accordingly.

For example:
		simple-audio-card,dai-link@0 {
			reg = <0>;
			bitclock-master = <&dailink0_master>;
			frame-master = <&dailink0_master>;
			format = "i2s";

			cpu {
				sound-dai = <&sai6>;
			};

			dailink0_master: codec {
				sound-dai = <&wm8904>;
				clocks = <&mclk>;
			};
		};

In this case mclk will be enabled and disabled accordingly.

Could you test this?

-michael
Sameer Pujar March 11, 2021, 11:05 a.m. UTC | #11
On 3/11/2021 3:57 PM, Michael Walle wrote:
> I've had a closer look at this and it seems you're messing around
> with the clock of the codec's node (which is _not_ a subnode of
> the simple-audio-card). I don't think this is correct.
>
> I guess you should rather set the clock property in the codec
> subnode of the simple-audio-card, which is then picked up by the
> simple-audio-card driver and changed accordingly.
>
> For example:
>                simple-audio-card,dai-link@0 {
>                        reg = <0>;
>                        bitclock-master = <&dailink0_master>;
>                        frame-master = <&dailink0_master>;
>                        format = "i2s";
>
>                        cpu {
>                                sound-dai = <&sai6>;
>                        };
>
>                        dailink0_master: codec {
>                                sound-dai = <&wm8904>;
>                                clocks = <&mclk>;
>                        };
>                };
>
> In this case mclk will be enabled and disabled accordingly.
>
> Could you test this?
>

It would work and initially I had similar patch, see [0] and related 
series. Suggestion is to always use "clocks" property with devices only.


[0] 
https://patchwork.kernel.org/project/alsa-devel/patch/1611944866-29373-4-git-send-email-spujar@nvidia.com/
Michael Walle March 11, 2021, 11:16 a.m. UTC | #12
Am 2021-03-11 12:05, schrieb Sameer Pujar:

> It would work and initially I had similar patch, see [0] and related
> series. Suggestion is to always use "clocks" property with devices
> only.

I see. But again, I don't think it is correct to change the clock of
the codec by default. What happens if this is for example a
compatible = "fixed-clock"?

As you pointed out in the referred thread [0]. simple-audio-card has
that clock and judging from the code it is exactly for this reason:
to either change/enable it or not.

With this patch you'll switch that to "always change it". Therefore,
shouldn't there be a dt flag to indicate wheter simple-audio-card/graph
should be in charge of the codecs clock input?

And its fetching just the first clock, doesn't it? What happens if a
codec has two clock inputs?

-michael

[0] 
https://patchwork.kernel.org/project/alsa-devel/patch/1611944866-29373-4-git-send-email-spujar@nvidia.com/
Sameer Pujar March 11, 2021, 2:29 p.m. UTC | #13
On 3/11/2021 4:46 PM, Michael Walle wrote:
> Am 2021-03-11 12:05, schrieb Sameer Pujar:
>
>> It would work and initially I had similar patch, see [0] and related
>> series. Suggestion is to always use "clocks" property with devices
>> only.
>
> I see. But again, I don't think it is correct to change the clock of
> the codec by default. What happens if this is for example a
> compatible = "fixed-clock"?

The codec rate won't be changed unless a corresponding "*mclk-fs" is 
provided.

>
> As you pointed out in the referred thread [0]. simple-audio-card has
> that clock and judging from the code it is exactly for this reason:
> to either change/enable it or not.
>


> With this patch you'll switch that to "always change it". Therefore,
> shouldn't there be a dt flag to indicate wheter simple-audio-card/graph
> should be in charge of the codecs clock input?

As mentioned above, it does not change always. Requires "*mclk-fs" to do so.

May be below could be a possible alternative?
- Re-order if-else of clock parsing.

    if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
        // Since you are fixing rate already via "assigned-clocks" this 
may be a duplication. OR
        // "assigned-clocks" can be parsed to understand if a fixed rate 
is expected.

        simple_dai->sysclk = val;
    } else {
        // fetch MCLK clock from device and setup sysclk
        // a. If "*mclk-fs" is given and "clocks" is found, the rate 
would be updated.
        // b. If "*mclk-fs" is not mentioned and "clocks" is found, then 
simple-card utils won't touch rate. It will just do clock enable/disable.
    }

>
> And its fetching just the first clock, doesn't it? What happens if a
> codec has two clock inputs?

Yes, it would have been more descriptive if it were specifically looking 
for clock "mclk". I think the original assumption was codec takes one 
input clock (MCLK) and uses it for sysclk.

>
> -michael
>
> [0]
> https://patchwork.kernel.org/project/alsa-devel/patch/1611944866-29373-4-git-send-email-spujar@nvidia.com/ 
>
Michael Walle March 11, 2021, 3:43 p.m. UTC | #14
Am 2021-03-11 15:29, schrieb Sameer Pujar:
> On 3/11/2021 4:46 PM, Michael Walle wrote:
>> Am 2021-03-11 12:05, schrieb Sameer Pujar:
>> 
>>> It would work and initially I had similar patch, see [0] and related
>>> series. Suggestion is to always use "clocks" property with devices
>>> only.
>> 
>> I see. But again, I don't think it is correct to change the clock of
>> the codec by default. What happens if this is for example a
>> compatible = "fixed-clock"?
> 
> The codec rate won't be changed unless a corresponding "*mclk-fs" is 
> provided.
> 
>> 
>> As you pointed out in the referred thread [0]. simple-audio-card has
>> that clock and judging from the code it is exactly for this reason:
>> to either change/enable it or not.
>> 
> 
> 
>> With this patch you'll switch that to "always change it". Therefore,
>> shouldn't there be a dt flag to indicate wheter 
>> simple-audio-card/graph
>> should be in charge of the codecs clock input?
> 
> As mentioned above, it does not change always. Requires "*mclk-fs" to 
> do so.

As mentioned earlier, this is changing the sysclk, too. And I'd guess
most codecs need a fixed factor for the sysclk, thus if the codec 
supports
generating its own sysclk by a PLL it will need this factor, too.

Which is also defined in the binding:

   system-clock-frequency:
     description: |
       If a clock is specified and a multiplication factor is given with
       mclk-fs, the clock will be set to the calculated mclk frequency
       when the stream starts.


> May be below could be a possible alternative?
> - Re-order if-else of clock parsing.
> 
>    if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>        // Since you are fixing rate already via "assigned-clocks" this
> may be a duplication. OR

exactly. and also need a device tree change (also for older kernels)
on my side.

This could be a last resort, yes. But I'd rather see a flag which
indicates whether the simple-audio-card should control the (first)
clock of the codec or not. Because until now, this wasn't the case.
And I don't know if this was an oversight or on purpose. Kuninori would
need to comment on that. And with the "we change mclk by default", we
break codecs with automatic sysclk generation.

>        // "assigned-clocks" can be parsed to understand if a fixed
> rate is expected.

Sounds like a hack to me. Esp. its doing the same as its already
doing right now. That is a "sysfreq = clk_get_rate(codec)".

>        simple_dai->sysclk = val;
>    } else {
>        // fetch MCLK clock from device and setup sysclk
>        // a. If "*mclk-fs" is given and "clocks" is found, the rate
> would be updated.
>        // b. If "*mclk-fs" is not mentioned and "clocks" is found,
> then simple-card utils won't touch rate. It will just do clock
> enable/disable.

mclk-fs is also a factor for the sysclk, thus it is also needed
if there is no mclk (or a fixed mclk).

I don't think you can deduce whether you can change the codecs
first clock with the current information :(

>    }
> 
>> 
>> And its fetching just the first clock, doesn't it? What happens if a
>> codec has two clock inputs?
> 
> Yes, it would have been more descriptive if it were specifically
> looking for clock "mclk". I think the original assumption was codec
> takes one input clock (MCLK) and uses it for sysclk.

Yeah, I've just noticed that the clk_get_rate() also only works
for the first clock of the codec.

-michael
Mark Brown March 11, 2021, 4 p.m. UTC | #15
On Tue, Mar 09, 2021 at 03:41:56PM +0100, Michael Walle wrote:
> Hi,
> 
> > If "clocks = <&xxx>" is specified from the CPU or Codec component
> > device node, the clock is not getting enabled. Thus audio playback
> > or capture fails.

> This actually breaks sound on my board
> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).

Please, when sending replies format the subject line like normal replies
with a "Re: " at the start so people can tell it's a reply to an
existing discussion and not a new patch.
Mark Brown March 11, 2021, 4:15 p.m. UTC | #16
On Wed, Mar 10, 2021 at 08:20:28PM +0530, Sameer Pujar wrote:

> If I read this correctly below is the configuration you need,
> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> sysclk

For this device for integration with something like simple-audio-card
since there's limited flexibility within the device the simplest thing
would be to not make the internal clocking of the device visible and
just have it figure out how to use the input clock, using the MCLK
directly if possible otherwise using the FLL to generate a suitable
clock.  The trick is figuring out if it's best to vary the input clock
or to use the FLL to adapt a fixed input clock, and of course adapting
any existing users if things get changed.
Mark Brown March 11, 2021, 4:41 p.m. UTC | #17
On Thu, Mar 11, 2021 at 04:43:20PM +0100, Michael Walle wrote:

> This could be a last resort, yes. But I'd rather see a flag which
> indicates whether the simple-audio-card should control the (first)
> clock of the codec or not. Because until now, this wasn't the case.
> And I don't know if this was an oversight or on purpose. Kuninori would
> need to comment on that. And with the "we change mclk by default", we
> break codecs with automatic sysclk generation.

It shouldn't break anything so long as the clock ends up correct via
some path.  Where there's multiple options we can also try going through
them in some order, preferring the clock in the CODEC would probably
make sense from both a compatibility and quality point of view.

> > > And its fetching just the first clock, doesn't it? What happens if a
> > > codec has two clock inputs?

> > Yes, it would have been more descriptive if it were specifically
> > looking for clock "mclk". I think the original assumption was codec
> > takes one input clock (MCLK) and uses it for sysclk.

> Yeah, I've just noticed that the clk_get_rate() also only works
> for the first clock of the codec.

simple-audio-card isn't really intended to work with complex devices,
it's very much only for the simplest of use cases.
Michael Walle March 11, 2021, 9:34 p.m. UTC | #18
Am 2021-03-11 17:00, schrieb Mark Brown:
> On Tue, Mar 09, 2021 at 03:41:56PM +0100, Michael Walle wrote:
>> Hi,
>> 
>> > If "clocks = <&xxx>" is specified from the CPU or Codec component
>> > device node, the clock is not getting enabled. Thus audio playback
>> > or capture fails.
> 
>> This actually breaks sound on my board
>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
> 
> Please, when sending replies format the subject line like normal 
> replies
> with a "Re: " at the start so people can tell it's a reply to an
> existing discussion and not a new patch.

Whoops, must have missed that. I need to figure out a new mail setup,
rather than saving the mbox from lore.kernel.org, editing it and
sending it with git-send-email.

-michael
Michael Walle March 11, 2021, 10:11 p.m. UTC | #19
Am 2021-03-11 17:15, schrieb Mark Brown:
> On Wed, Mar 10, 2021 at 08:20:28PM +0530, Sameer Pujar wrote:
> 
>> If I read this correctly below is the configuration you need,
>> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> 
>> sysclk
> 
> For this device for integration with something like simple-audio-card
> since there's limited flexibility within the device the simplest thing
> would be to not make the internal clocking of the device visible and
> just have it figure out how to use the input clock, using the MCLK
> directly if possible otherwise using the FLL to generate a suitable
> clock.

Before this patch, part of this was already happening. That is,
simple-audio-card called set_sysclk(samplerate * mclk-fs), then
the codec figured out that its mclk was different than the
requested sample rate and enabled its FLL to generate the requested
sample rate automatically.

With this patch applied, simple-audio-card already tries to change
mclk, which isn't working in my case (the MCLK isn't generated by
a PLL and just supports fixed frequencies) and thus breaks audio. And
this patch also propagate to the stable kernels and breaks my
board there, too.

> The trick is figuring out if it's best to vary the input clock
> or to use the FLL to adapt a fixed input clock,

For simple-audio-card you can set the "clock" property if you want
that clock to be changed/enabled/disabled. But that doesn't seem to
be the way to go, at least it was NAKed by Rob for the audio-graph-card.
I don't see a way to figure out if MCLK should be controlled by
simple-*-card without adding further properties to the device tree.

> and of course adapting any existing users if things get changed.

To be frank, I don't see that happening.

-michael
Michael Walle March 12, 2021, 12:01 p.m. UTC | #20
Am 2021-03-12 12:35, schrieb Mark Brown:
> On Thu, Mar 11, 2021 at 11:11:15PM +0100, Michael Walle wrote:
>> Am 2021-03-11 17:15, schrieb Mark Brown:
> 
>> > The trick is figuring out if it's best to vary the input clock
>> > or to use the FLL to adapt a fixed input clock,
> 
>> For simple-audio-card you can set the "clock" property if you want
>> that clock to be changed/enabled/disabled. But that doesn't seem to
>> be the way to go, at least it was NAKed by Rob for the 
>> audio-graph-card.
>> I don't see a way to figure out if MCLK should be controlled by
>> simple-*-card without adding further properties to the device tree.
> 
> If the card has a clock API clock as sysclk then set_sysclk(() should
> be configuring that clock.

What do you mean by "the card". The simple-audio-card itself?

Take a look at:
https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts#L29

Does the card has a clock? IMHO the WM8904 codec has a clock, but not
the audio card.

-michael
Michael Walle March 12, 2021, 12:30 p.m. UTC | #21
Am 2021-03-12 13:04, schrieb Mark Brown:
> On Fri, Mar 12, 2021 at 01:01:41PM +0100, Michael Walle wrote:
>> Am 2021-03-12 12:35, schrieb Mark Brown:
> 
>> > If the card has a clock API clock as sysclk then set_sysclk(() should
>> > be configuring that clock.
> 
>> What do you mean by "the card". The simple-audio-card itself?
> 
>> Take a look at:
>> https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts#L29
> 
>> Does the card has a clock? IMHO the WM8904 codec has a clock, but not
>> the audio card.
> 
> The clock on the CODEC, which the card configures.  The CODEC should be
> passing on the configuration to the clock API.

Sorry, I don't understand.

The card calls set_sysclk(), which eventually ends up in the codec.
The codec therefore, could figure out if it needs to configure the
clock or if it can use its internal FLL.
Is that what you mean?

But the set_sysclk() of the codec isn't even called, because the
card itself already tries to call clk_set_rate() on the Codec's MCLK,
which returns with an error [0].

[0] 
https://elixir.bootlin.com/linux/v5.12-rc2/source/sound/soc/generic/simple-card-utils.c#L265

-michael
Mark Brown March 12, 2021, 1:46 p.m. UTC | #22
On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:

> The card calls set_sysclk(), which eventually ends up in the codec.
> The codec therefore, could figure out if it needs to configure the
> clock or if it can use its internal FLL.
> Is that what you mean?

Yes.

> But the set_sysclk() of the codec isn't even called, because the
> card itself already tries to call clk_set_rate() on the Codec's MCLK,
> which returns with an error [0].

OK, so I think we need to push this down a level so that the clock
setting is implemented by the core/CODEC rather than by simple-card,
with the helpers being something the CODEC can opt out of.
Michael Walle March 15, 2021, 12:05 p.m. UTC | #23
Am 2021-03-12 14:46, schrieb Mark Brown:
> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
> 
>> The card calls set_sysclk(), which eventually ends up in the codec.
>> The codec therefore, could figure out if it needs to configure the
>> clock or if it can use its internal FLL.
>> Is that what you mean?
> 
> Yes.
> 
>> But the set_sysclk() of the codec isn't even called, because the
>> card itself already tries to call clk_set_rate() on the Codec's MCLK,
>> which returns with an error [0].
> 
> OK, so I think we need to push this down a level so that the clock
> setting is implemented by the core/CODEC rather than by simple-card,
> with the helpers being something the CODEC can opt out of.

Sameer, it looks like the proper fix should be to add the clock
support to your codec.

I've also looked at other users of "simple-audio-card" and
it looks like they will break too. For example,
- arch/arm64/boot/dts/rockchip/rk3399.dtsi
     If I'm not mistaken, this will try to set the first clock
     of hdmi@ff940000 there, which is "iahb".
- arch/arm/boot/dts/sun8i-a33.dtsi
     There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed

And it doesn't stop there, it also sets the first clock
of the CPU endpoint, which I guess just works because .set_rate
is a noop for the most clocks which are used there.

-michael
Mark Brown March 15, 2021, 3:39 p.m. UTC | #24
On Mon, Mar 15, 2021 at 08:49:00PM +0530, Sameer Pujar wrote:
> On 3/15/2021 5:35 PM, Michael Walle wrote:
> > Am 2021-03-12 14:46, schrieb Mark Brown:

> > Sameer, it looks like the proper fix should be to add the clock
> > support to your codec.

> I agree that complicated clock relationships should be handled within the
> codec itself, however MCLK rate setting depends on "mclk-fs" factor and this
> property is specified as part of simple-card/audio-graph-card codec subnode.
> Right now codec, in general, does not have a way to know this. The
> set_sysclk() callback takes rate argument and not the factor. Moreover the

I just don't understand what you're saying here at all.  At the point
where the card is setting the clock API clock rate it can just as well
set a sysclk, these are equivalent operations.

> same codec is used by other platform vendors too and unless a new DT
> property is added for codec, runtime MCLK update based on the scaling factor
> cannot be supported. This would mean that we will be having two methods to
> specify "mclk-fs" factor, one from simple-card/audio-graph-card and one from
> respective codec nodes, which does not seem ideal.

Again I just can't follow what you're saying here at all.  Again, if the
card is able to set a clock API clock rate it can just as well set a
clock rate via sysclk.

> Yes this is a problem, unfortunately I missed checking some of the
> simple-card examples. I wonder we should be specifically looking for "mclk"
> clock here.

That would definitely help mitigate the problem but I really think it's
cleaner and safer to just push this down to set_sysclk().
Michael Walle March 15, 2021, 5:13 p.m. UTC | #25
Am 2021-03-15 18:10, schrieb Sameer Pujar:
>>> Yes this is a problem, unfortunately I missed checking some of the
>>> simple-card examples. I wonder we should be specifically looking for 
>>> "mclk"
>>> clock here.
>> That would definitely help mitigate the problem but I really think 
>> it's
>> cleaner and safer to just push this down to set_sysclk().
> 
> Understand now. I will push patches based on this. Thanks for the 
> details.

Please keep me on CC, I'm not subscribed to the sound/alsa mailinglist.

Thanks,
-michael
diff mbox series

Patch

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index bc0b62e..0754d70 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -173,16 +173,15 @@  int asoc_simple_parse_clk(struct device *dev,
 	 *  or device's module clock.
 	 */
 	clk = devm_get_clk_from_child(dev, node, NULL);
-	if (!IS_ERR(clk)) {
-		simple_dai->sysclk = clk_get_rate(clk);
+	if (IS_ERR(clk))
+		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
 
+	if (!IS_ERR(clk)) {
 		simple_dai->clk = clk;
-	} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
+		simple_dai->sysclk = clk_get_rate(clk);
+	} else if (!of_property_read_u32(node, "system-clock-frequency",
+					 &val)) {
 		simple_dai->sysclk = val;
-	} else {
-		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
-		if (!IS_ERR(clk))
-			simple_dai->sysclk = clk_get_rate(clk);
 	}
 
 	if (of_property_read_bool(node, "system-clock-direction-out"))