| Message ID | CAKON4Oz0VbZ=j-0YqkN=9C+yXbWycc8TvsdTCNH0UzuvrcXs_w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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 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 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
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;
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.