Choosing the sysclk in simple-card looks broken to me.

Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

The problem is in asoc_simple_card_sub_parse_of()
in the else case..
} else {
clk = of_clk_get(node, 0);
if (!IS_ERR(clk))
dai->sysclk = clk_get_rate(clk);
}

This is picking up the input clocks to the devices.  On the cpu-dai
you want to pick up the ouput mclk (my first input clock is an 80Mhz
bus clock).  My IIS output clock is: clock-output-names = "mclk0";

Setting clocks="" doesn't work either, it also picks up the input clock.

if (of_property_read_bool(np, "clocks")) {
  clk = of_clk_get(np, 0);
  if (IS_ERR(clk)) {
     ret = PTR_ERR(clk);
  return ret;

When the codec-dai bit runs it finds the input clock on the codec,
which is really my cpu-dai's output clock and it gets the correct
value.

But then simple goes and tries to set both system clocks - my 80Mhz
bus clock and the 22.5 true sysclk. And my cpu-dai barfs when it is
passed a 80Mhz sysclk.

How to fix? Is anyone using this feature?  This needs to be fixed to
pick up output clocks, not input ones.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1189 bytes --]

On Sat, Aug 09, 2014 at 08:08:45PM -0400, jonsmirl@gmail.com wrote:

> The problem is in asoc_simple_card_sub_parse_of()
> in the else case..

> } else {
> clk = of_clk_get(node, 0);
> if (!IS_ERR(clk))
> dai->sysclk = clk_get_rate(clk);
> }

> This is picking up the input clocks to the devices.  On the cpu-dai
> you want to pick up the ouput mclk (my first input clock is an 80Mhz
> bus clock).  My IIS output clock is: clock-output-names = "mclk0";

> Setting clocks="" doesn't work either, it also picks up the input clock.

I'm sorry but I'm having an awfully hard time working out what you are
trying to say in this mail.  When you say things like "you want to pick
up the ouput mclk" it's really not clear what any of that means - why do
you want to pick it and what is it being picked for?

> But then simple goes and tries to set both system clocks - my 80Mhz
> bus clock and the 22.5 true sysclk. And my cpu-dai barfs when it is
> passed a 80Mhz sysclk.

Naturally...  this does suggest you're doing something wrong but like I
say I really don't understand what's going on.  What does your system
look like, what are you trying to get it to do and how are you going
about that?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

On Sun, Aug 10, 2014 at 6:54 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Aug 09, 2014 at 08:08:45PM -0400, jonsmirl@gmail.com wrote:
>
>> The problem is in asoc_simple_card_sub_parse_of()
>> in the else case..
>
>> } else {
>> clk = of_clk_get(node, 0);
>> if (!IS_ERR(clk))
>> dai->sysclk = clk_get_rate(clk);
>> }
>
>> This is picking up the input clocks to the devices.  On the cpu-dai
>> you want to pick up the ouput mclk (my first input clock is an 80Mhz
>> bus clock).  My IIS output clock is: clock-output-names = "mclk0";
>
>> Setting clocks="" doesn't work either, it also picks up the input clock.
>
> I'm sorry but I'm having an awfully hard time working out what you are
> trying to say in this mail.  When you say things like "you want to pick
> up the ouput mclk" it's really not clear what any of that means - why do
> you want to pick it and what is it being picked for?

Inside simple audio card,  __asoc_simple_card_dai_init() calls
snd_soc_dai_set_sysclk(). So where is it getting the clock values used
in __asoc_simple_card_dai_init()?

Here is my DTS for my cpu-dai...

iis0: iis@01c22400 {
  #clock-cells = <0>;
  #sound-dai-cells = <0>;
  compatible = "allwinner,sun7i-a20-iis";
  reg = <0x01C22400 0x40>;
  interrupts = <0 16 4>;
  clocks = <&apb0_gates 3>, <&i2s0_clk>;
  clock-names = "apb", "iis";
  clock-output-names = "mclk0";
  dmas = <&dma 0 3>, <&dma 0 3>;
  dma-names = "rx", "tx";
  status = "disabled";
};

This unit has three clocks.
apb - bus input 80Mhz to run the unit
iis - a programable input from a system PLL
mclk0 - a clock that is output to the MCLK pin

This is my codec

sgtl5000: sgtl5000@a {
   compatible = "fsl,sgtl5000";
   reg = <0x0a>;
   clocks = <&iis0>;

   #sound-dai-cells = <0>;
   VDDA-supply = <&reg_vcc3v3>;
   VDDIO-supply = <&reg_vcc3v3>;
};

It has single input clock
clocks (unnamed) - it is wired to the MCLK pin

My simple audio card section...

sound {
  compatible = "simple-audio-card";
  simple-audio-card,format = "i2s";
  simple-audio-card,mclk-fs = <256>;

  simple-audio-card,cpu {
    sound-dai = <&iis0>;
  };

  simple-audio-card,codec {
    sound-dai = <&sgtl5000>;
   };
};

Now look at what simple_card_dai_link_of() does. It parse this section
and then calls asoc_simple_card_sub_parse_of() for each of the two
DAI's.

Now look look at what asoc_simple_card_sub_parse_of() does when it
parses each of those subnodes.  It looks up the first input clock off
from both of those nodes and saves it as dai->sysclk. Then in
__asoc_simple_card_dai_init() it pokes those values back into my
driver via snd_soc_dai_set_sysclk().

So on the first node.
 simple-audio-card,cpu {
    sound-dai = <&iis0>;
  };
It goes over to the iis driver and gets rate from the first input
clock for iis0 - apb (80Mhz) and sets this back into my driver via
snd_soc_dai_set_sysclk().

On the second node
  simple-audio-card,codec {
    sound-dai = <&sgtl5000>;
   };
Then it does a getrate() on the first input clock for sgtl5000. But
that clock has not been initialized yet. And then it sets the value
back in via snd_soc_dai_set_sysclk().

------

The problem is that asoc_simple_card_sub_parse_of() is doing getrate()
on input clocks when it should be doing it on output clocks.

So in my case, iis has a single output clock 'mclk0'. simple can
getrate() on it and then set the rate back in via
snd_soc_dai_set_sysclk(). Codec has no output clocks so it does
nothing with sysclk.





>
>> But then simple goes and tries to set both system clocks - my 80Mhz
>> bus clock and the 22.5 true sysclk. And my cpu-dai barfs when it is
>> passed a 80Mhz sysclk.
>
> Naturally...  this does suggest you're doing something wrong but like I
> say I really don't understand what's going on.  What does your system
> look like, what are you trying to get it to do and how are you going
> about that?



-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

This should fix it to use the output clocks, but does it break any of
the other DTS using simple-audio?

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 03a7fdc..8c4b267 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -116,6 +116,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 {
  struct device_node *node;
  struct clk *clk;
+ const char *clk_name = NULL;
  int ret;

  /*
@@ -156,11 +157,14 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
      "system-clock-frequency",
      &dai->sysclk);
  } else {
- clk = of_clk_get(node, 0);
- if (!IS_ERR(clk))
- dai->sysclk = clk_get_rate(clk);
+ of_property_read_string(node, "clock-output-names", &clk_name);
+ if (clk_name) {
+ clk = of_clk_get_by_name(node, clk_name);
+ if (!IS_ERR(clk)) {
+ dai->sysclk = clk_get_rate(clk);
+ }
+ }
  }
-
  return 0;
 }


On Sun, Aug 10, 2014 at 8:34 AM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Sun, Aug 10, 2014 at 6:54 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Sat, Aug 09, 2014 at 08:08:45PM -0400, jonsmirl@gmail.com wrote:
>>
>>> The problem is in asoc_simple_card_sub_parse_of()
>>> in the else case..
>>
>>> } else {
>>> clk = of_clk_get(node, 0);
>>> if (!IS_ERR(clk))
>>> dai->sysclk = clk_get_rate(clk);
>>> }
>>
>>> This is picking up the input clocks to the devices.  On the cpu-dai
>>> you want to pick up the ouput mclk (my first input clock is an 80Mhz
>>> bus clock).  My IIS output clock is: clock-output-names = "mclk0";
>>
>>> Setting clocks="" doesn't work either, it also picks up the input clock.
>>
>> I'm sorry but I'm having an awfully hard time working out what you are
>> trying to say in this mail.  When you say things like "you want to pick
>> up the ouput mclk" it's really not clear what any of that means - why do
>> you want to pick it and what is it being picked for?
>
> Inside simple audio card,  __asoc_simple_card_dai_init() calls
> snd_soc_dai_set_sysclk(). So where is it getting the clock values used
> in __asoc_simple_card_dai_init()?
>
> Here is my DTS for my cpu-dai...
>
> iis0: iis@01c22400 {
>   #clock-cells = <0>;
>   #sound-dai-cells = <0>;
>   compatible = "allwinner,sun7i-a20-iis";
>   reg = <0x01C22400 0x40>;
>   interrupts = <0 16 4>;
>   clocks = <&apb0_gates 3>, <&i2s0_clk>;
>   clock-names = "apb", "iis";
>   clock-output-names = "mclk0";
>   dmas = <&dma 0 3>, <&dma 0 3>;
>   dma-names = "rx", "tx";
>   status = "disabled";
> };
>
> This unit has three clocks.
> apb - bus input 80Mhz to run the unit
> iis - a programable input from a system PLL
> mclk0 - a clock that is output to the MCLK pin
>
> This is my codec
>
> sgtl5000: sgtl5000@a {
>    compatible = "fsl,sgtl5000";
>    reg = <0x0a>;
>    clocks = <&iis0>;
>
>    #sound-dai-cells = <0>;
>    VDDA-supply = <&reg_vcc3v3>;
>    VDDIO-supply = <&reg_vcc3v3>;
> };
>
> It has single input clock
> clocks (unnamed) - it is wired to the MCLK pin
>
> My simple audio card section...
>
> sound {
>   compatible = "simple-audio-card";
>   simple-audio-card,format = "i2s";
>   simple-audio-card,mclk-fs = <256>;
>
>   simple-audio-card,cpu {
>     sound-dai = <&iis0>;
>   };
>
>   simple-audio-card,codec {
>     sound-dai = <&sgtl5000>;
>    };
> };
>
> Now look at what simple_card_dai_link_of() does. It parse this section
> and then calls asoc_simple_card_sub_parse_of() for each of the two
> DAI's.
>
> Now look look at what asoc_simple_card_sub_parse_of() does when it
> parses each of those subnodes.  It looks up the first input clock off
> from both of those nodes and saves it as dai->sysclk. Then in
> __asoc_simple_card_dai_init() it pokes those values back into my
> driver via snd_soc_dai_set_sysclk().
>
> So on the first node.
>  simple-audio-card,cpu {
>     sound-dai = <&iis0>;
>   };
> It goes over to the iis driver and gets rate from the first input
> clock for iis0 - apb (80Mhz) and sets this back into my driver via
> snd_soc_dai_set_sysclk().
>
> On the second node
>   simple-audio-card,codec {
>     sound-dai = <&sgtl5000>;
>    };
> Then it does a getrate() on the first input clock for sgtl5000. But
> that clock has not been initialized yet. And then it sets the value
> back in via snd_soc_dai_set_sysclk().
>
> ------
>
> The problem is that asoc_simple_card_sub_parse_of() is doing getrate()
> on input clocks when it should be doing it on output clocks.
>
> So in my case, iis has a single output clock 'mclk0'. simple can
> getrate() on it and then set the rate back in via
> snd_soc_dai_set_sysclk(). Codec has no output clocks so it does
> nothing with sysclk.
>
>
>
>
>
>>
>>> But then simple goes and tries to set both system clocks - my 80Mhz
>>> bus clock and the 22.5 true sysclk. And my cpu-dai barfs when it is
>>> passed a 80Mhz sysclk.
>>
>> Naturally...  this does suggest you're doing something wrong but like I
>> say I really don't understand what's going on.  What does your system
>> look like, what are you trying to get it to do and how are you going
>> about that?
>
>
>
> --
> Jon Smirl
> jonsmirl@gmail.com



-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

After much messing around and learning the internals of OF clocks, it
looks like this is what I need.

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 03a7fdc..05d074b 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 #include <linux/string.h>
 #include <sound/simple_card.h>
 #include <sound/soc-dai.h>
@@ -116,6 +117,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 {
  struct device_node *node;
  struct clk *clk;
+ struct of_phandle_args clkspec;
  int ret;

  /*
@@ -156,11 +158,14 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
      "system-clock-frequency",
      &dai->sysclk);
  } else {
- clk = of_clk_get(node, 0);
- if (!IS_ERR(clk))
+ clkspec.np = node;
+ clk = of_clk_get_from_provider(&clkspec);
+
+ if (!IS_ERR(clk)) {
  dai->sysclk = clk_get_rate(clk);
+ clk_put(clk);
+ }
  }
-
  return 0;
 }




-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

As I debug further I am getting a better understanding of the problem.
This is my simple-audio binding.

sound {
compatible = "simple-audio-card";
simple-audio-card,format = "i2s";

simple-audio-card,cpu {
sound-dai = <&iis0>;
};

simple-audio-card,codec {
clocks = <&iis0>;
sound-dai = <&sgtl5000>;
};
};

I'm having trouble with the simple-audio-card,cpu node, not the
simple-audio-card,codec node.  As simple-audio is currently
implemented when simple-audio-card,cpu is processed it will pick up my
80Mhz apb clock and set it as sysclk.

After spending all day trying to fix that else clause, I'm coming to
the conclusion that the else clause simply shouldn't be there at all.
I just hit this because I had code checking for bogus values in
setsysclk.

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 03a7fdc..b389d9c2 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -155,10 +155,6 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
  of_property_read_u32(np,
      "system-clock-frequency",
      &dai->sysclk);
- } else {
- clk = of_clk_get(node, 0);
- if (!IS_ERR(clk))
- dai->sysclk = clk_get_rate(clk);
  }

  return 0;


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[Nicolin Chen]

On Sun, Aug 10, 2014 at 10:10:51PM -0400, jonsmirl@gmail.com wrote:
> As I debug further I am getting a better understanding of the problem.
> This is my simple-audio binding.
> 
> sound {
> compatible = "simple-audio-card";
> simple-audio-card,format = "i2s";
> 
> simple-audio-card,cpu {
> sound-dai = <&iis0>;
> };
> 
> simple-audio-card,codec {
> clocks = <&iis0>;
> sound-dai = <&sgtl5000>;
> };
> };
> 
> I'm having trouble with the simple-audio-card,cpu node, not the
> simple-audio-card,codec node.  As simple-audio is currently
> implemented when simple-audio-card,cpu is processed it will pick up my
> 80Mhz apb clock and set it as sysclk.

It looks like the problem is that Simple Card also configures cpu-dai
via set_sysclk() with direction SND_SOC_CLOCK_IN while it fetches the
sysclk rate not from MCLK source but from 'sound-dai' -- cpu-dai node
itself, when the binding for sound-dai doesn't have clock or frequency
assignment, although this fetching works out for codec-dai side.

In your case, Simple Card sets both sides of dai with different sysclk
rates, and meanwhile the sysclk rate for cpu-dai is incorrect.

Is it true?

> After spending all day trying to fix that else clause, I'm coming to
> the conclusion that the else clause simply shouldn't be there at all.
> I just hit this because I had code checking for bogus values in
> setsysclk.

Well, even if there's a problem at this point for Simple Card, I'm
still curious for the reason why setting cpu-dai with an incorrect
rate but direction SND_SOC_CLOCK_IN would result some breaks here.

The sysclk direction is only valid when indicating MCLK as sysclk
(FSYNC or BCLK shall follow CBS_CFS/CBM_CFM setting).So basically
if cpu-dai driver gets sysclk configuration with SND_SOC_CLOCK_IN,
shouldn't it just set the internal direction for MCLK's PAD and
return the call directly? (I'm guessing there also might be some
tiny work for your cpu-dai driver to enhance the validation part.)

Best regards,
Nicolin

Re: Choosing the sysclk in simple-card looks broken to me.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1121 bytes --]

On Sun, Aug 10, 2014 at 02:42:23PM -0400, jonsmirl@gmail.com wrote:
> This should fix it to use the output clocks, but does it break any of
> the other DTS using simple-audio?
> 
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 03a7fdc..8c4b267 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -116,6 +116,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  {
>   struct device_node *node;
>   struct clk *clk;
> + const char *clk_name = NULL;
>   int ret;
> 
>   /*
> @@ -156,11 +157,14 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>       "system-clock-frequency",
>       &dai->sysclk);
>   } else {
> - clk = of_clk_get(node, 0);
> - if (!IS_ERR(clk))
> - dai->sysclk = clk_get_rate(clk);
> + of_property_read_string(node, "clock-output-names", &clk_name);
> + if (clk_name) {
> + clk = of_clk_get_by_name(node, clk_name);
> + if (!IS_ERR(clk)) {
> + dai->sysclk = clk_get_rate(clk);
> + }

Jon, please use indentation when sending code via e-mail - this is far
too hard to read.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Choosing the sysclk in simple-card looks broken to me.

[Nicolin Chen]

On Mon, Aug 11, 2014 at 07:56:12AM -0400, jonsmirl@gmail.com wrote:
> > It looks like the problem is that Simple Card also configures cpu-dai
> > via set_sysclk() with direction SND_SOC_CLOCK_IN while it fetches the
> > sysclk rate not from MCLK source but from 'sound-dai' -- cpu-dai node
> > itself, when the binding for sound-dai doesn't have clock or frequency
> > assignment, although this fetching works out for codec-dai side.
> >
> > In your case, Simple Card sets both sides of dai with different sysclk
> > rates, and meanwhile the sysclk rate for cpu-dai is incorrect.
> >
> > Is it true?
> 
> Yes, that is what is going on.  Trying to use sgtl5000 with
> simple-audio exposed that issue. Without removing that 'else' clause
> there is no way to tell simple-audio just to leave the cpu-dai alone.

This issue remained in Simple Card shall be fixed. But I don't think
it's a good idea to _just_ remove that 'else' since its does work for
codec-dai. Instead, I think you might need to check the set_sysclk()
of cpu-dai driver as I mentioned in the last mail:

 The sysclk direction is only valid when indicating MCLK as sysclk
 (FSYNC or BCLK shall follow CBS_CFS/CBM_CFM setting).So basically
 if cpu-dai driver gets sysclk configuration with SND_SOC_CLOCK_IN,
 shouldn't it just set the internal direction for MCLK's PAD and
 return the call directly? (I'm guessing there also might be some
 tiny work for your cpu-dai driver to enhance the validation part.)

Best regards,
Nicolin

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

On Mon, Aug 11, 2014 at 3:28 AM, Nicolin Chen
<Guangyu.Chen@freescale.com> wrote:
> On Sun, Aug 10, 2014 at 10:10:51PM -0400, jonsmirl@gmail.com wrote:
>> As I debug further I am getting a better understanding of the problem.
>> This is my simple-audio binding.
>>
>> sound {
>> compatible = "simple-audio-card";
>> simple-audio-card,format = "i2s";
>>
>> simple-audio-card,cpu {
>> sound-dai = <&iis0>;
>> };
>>
>> simple-audio-card,codec {
>> clocks = <&iis0>;
>> sound-dai = <&sgtl5000>;
>> };
>> };
>>
>> I'm having trouble with the simple-audio-card,cpu node, not the
>> simple-audio-card,codec node.  As simple-audio is currently
>> implemented when simple-audio-card,cpu is processed it will pick up my
>> 80Mhz apb clock and set it as sysclk.
>
> It looks like the problem is that Simple Card also configures cpu-dai
> via set_sysclk() with direction SND_SOC_CLOCK_IN while it fetches the
> sysclk rate not from MCLK source but from 'sound-dai' -- cpu-dai node
> itself, when the binding for sound-dai doesn't have clock or frequency
> assignment, although this fetching works out for codec-dai side.
>
> In your case, Simple Card sets both sides of dai with different sysclk
> rates, and meanwhile the sysclk rate for cpu-dai is incorrect.
>
> Is it true?

Yes, that is what is going on.  Trying to use sgtl5000 with
simple-audio exposed that issue. Without removing that 'else' clause
there is no way to tell simple-audio just to leave the cpu-dai alone.

>
>> After spending all day trying to fix that else clause, I'm coming to
>> the conclusion that the else clause simply shouldn't be there at all.
>> I just hit this because I had code checking for bogus values in
>> setsysclk.
>
> Well, even if there's a problem at this point for Simple Card, I'm
> still curious for the reason why setting cpu-dai with an incorrect
> rate but direction SND_SOC_CLOCK_IN would result some breaks here.
>
> The sysclk direction is only valid when indicating MCLK as sysclk
> (FSYNC or BCLK shall follow CBS_CFS/CBM_CFM setting).So basically
> if cpu-dai driver gets sysclk configuration with SND_SOC_CLOCK_IN,
> shouldn't it just set the internal direction for MCLK's PAD and
> return the call directly? (I'm guessing there also might be some
> tiny work for your cpu-dai driver to enhance the validation part.)
>
> Best regards,
> Nicolin



-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

On Mon, Aug 11, 2014 at 6:48 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Aug 10, 2014 at 02:42:23PM -0400, jonsmirl@gmail.com wrote:
>> This should fix it to use the output clocks, but does it break any of
>> the other DTS using simple-audio?
>>
>> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
>> index 03a7fdc..8c4b267 100644
>> --- a/sound/soc/generic/simple-card.c
>> +++ b/sound/soc/generic/simple-card.c
>> @@ -116,6 +116,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>>  {
>>   struct device_node *node;
>>   struct clk *clk;
>> + const char *clk_name = NULL;
>>   int ret;
>>
>>   /*
>> @@ -156,11 +157,14 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>>       "system-clock-frequency",
>>       &dai->sysclk);
>>   } else {
>> - clk = of_clk_get(node, 0);
>> - if (!IS_ERR(clk))
>> - dai->sysclk = clk_get_rate(clk);
>> + of_property_read_string(node, "clock-output-names", &clk_name);
>> + if (clk_name) {
>> + clk = of_clk_get_by_name(node, clk_name);
>> + if (!IS_ERR(clk)) {
>> + dai->sysclk = clk_get_rate(clk);
>> + }
>
> Jon, please use indentation when sending code via e-mail - this is far
> too hard to read.

Seems that gmail is eating my tabs. I'll see if I can set the my
editor to convert them to spaces before pasting.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Choosing the sysclk in simple-card looks broken to me.

[jonsmirl]

On Mon, Aug 11, 2014 at 7:49 AM, Nicolin Chen
<Guangyu.Chen@freescale.com> wrote:
> On Mon, Aug 11, 2014 at 07:56:12AM -0400, jonsmirl@gmail.com wrote:
>> > It looks like the problem is that Simple Card also configures cpu-dai
>> > via set_sysclk() with direction SND_SOC_CLOCK_IN while it fetches the
>> > sysclk rate not from MCLK source but from 'sound-dai' -- cpu-dai node
>> > itself, when the binding for sound-dai doesn't have clock or frequency
>> > assignment, although this fetching works out for codec-dai side.
>> >
>> > In your case, Simple Card sets both sides of dai with different sysclk
>> > rates, and meanwhile the sysclk rate for cpu-dai is incorrect.
>> >
>> > Is it true?
>>
>> Yes, that is what is going on.  Trying to use sgtl5000 with
>> simple-audio exposed that issue. Without removing that 'else' clause
>> there is no way to tell simple-audio just to leave the cpu-dai alone.
>
> This issue remained in Simple Card shall be fixed. But I don't think
> it's a good idea to _just_ remove that 'else' since its does work for
> codec-dai. Instead, I think you might need to check the set_sysclk()
> of cpu-dai driver as I mentioned in the last mail:

The code in that else case is just getting lucky. It is picking up the
rate from the first input clock defined in the DTS for the device.

For my cpu-dai the first input clock is the apb bus clock which is
running at 80Mhz. In this case the default did the totally wrong
thing. On my CPU you really can set the sysclk to 80Mhz and that way
exceeds the input spec for sgtl5000. I just happen to notice because I
had some debug code telling what clock rates were being set.

For the sgtl5000 the first input clock happens to be the link back to the codec.

So maybe the fix is to only run that else case for for the codec-dai?


>
>  The sysclk direction is only valid when indicating MCLK as sysclk
>  (FSYNC or BCLK shall follow CBS_CFS/CBM_CFM setting).So basically
>  if cpu-dai driver gets sysclk configuration with SND_SOC_CLOCK_IN,
>  shouldn't it just set the internal direction for MCLK's PAD and
>  return the call directly? (I'm guessing there also might be some
>  tiny work for your cpu-dai driver to enhance the validation part.)
>
> Best regards,
> Nicolin



-- 
Jon Smirl
jonsmirl@gmail.com