[alsa-devel] Choosing the sysclk in simple-card looks broken to me.
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.
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?
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 = <®_vcc3v3>; VDDIO-supply = <®_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?
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 = <®_vcc3v3>; VDDIO-supply = <®_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
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; }
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;
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
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
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
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
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.
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.
participants (3)
-
jonsmirl＠gmail.com -
Mark Brown -
Nicolin Chen