diff mbox

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

Message ID CAKON4OxvoLBiq64t6Oh1i3CgA=Zy5DU7xYEe2kkpVdWkXMmqsg@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jon Smirl Aug. 10, 2014, 6:42 p.m. UTC 
This should fix it to use the output clocks, but does it break any of
the other DTS using simple-audio?


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

Comments

Mark Brown Aug. 11, 2014, 10:48 a.m. UTC | #1 
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.
Jon Smirl Aug. 11, 2014, 12:04 p.m. UTC | #2 
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.
diff mbox

Patch

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;
 }