| Message ID | AM5PR0401MB262706674539BFE651706CC6E3900@AM5PR0401MB2627.eurprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Fabio, > [Sorry for the top-posting] > > > The driver currently has: > > > /* > * Hardware limitation: The bclk rate must be > * never greater than 1/5 IPG clock rate > */ > if (freq * 5 > clk_get_rate(ssi_private->clk)) { > dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); > return -EINVAL; > } > Unfortunately not. This is the part of fsl_ssi_set_bclk() function which is called after fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;). Before the aforementioned check we do have: if (ssi_private->bitclk_freq) freq = ssi_private->bitclk_freq; else freq = params_channels(hw_params) * 32 * params_rate(hw_params); Which assigns freq = bitclk_freq (66 MHz) And then we break on this particular check: 66MHz * 5 > 66 MHz. The culprit IMHO is the ssi_private->bitclk_freq = freq; in the fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock (ssi_private->clk), not the bit clock (BCLK). This patch just quits early if it detects change, which don't need to be done. > > Isn't this properly taking care of the clock restriction? > > ________________________________ > From: Lukasz Majewski <lukma@denx.de> > Sent: Sunday, September 3, 2017 8:05:01 AM > To: Timur Tabi; Nicolin Chen; Xiubo Li; Fabio Estevam; Liam Girdwood; Mark Brown; Jaroslav Kysela; Takashi Iwai > Cc: alsa-devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Lukasz Majewski > Subject: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq > > The problem is visible in the following setup (on the imx6q): > "simple-audio-card" -> ssi2 -> I2S + I2C -> codec > > The function call log (simple-card probe -> CONFIG_SND_SIMPLE_CARD): > > asoc_simple_card_init_dai() @ sound/soc/generic/simple-card-utils.c > snd_soc_dai_set_sysclk() > fsl_ssi_set_dai_sysclk() @ sound/soc/fsl/fsl_ssi.c > > The last call is changing the bit clock (BCLK) frequency to SSI's IP > block clock (ipg = 66 MHz) [1]. > This is wrong, since IMX SSI block requires the I2S BCLK to be less > than 1/5 of [1]. > > As a result the driver initialization passes without any errors, but the > speaker-test test case breaks. > > This commit checks if the fsl_ssi_set_dai_sysclk() frequency passed is > not equal to [1]. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > sound/soc/fsl/fsl_ssi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > index 173cb84..1186fa9 100644 > --- a/sound/soc/fsl/fsl_ssi.c > +++ b/sound/soc/fsl/fsl_ssi.c > @@ -809,6 +809,8 @@ static int fsl_ssi_set_dai_sysclk(struct snd_soc_dai *cpu_dai, > int clk_id, unsigned int freq, int dir) > { > struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai); > + if (clk_get_rate(ssi_private->clk) == freq) > + return 0; > > ssi_private->bitclk_freq = freq; > > -- > 2.1.4 > >
On Sun, Sep 3, 2017 at 11:40 AM, Łukasz Majewski <lukma@denx.de> wrote: > This is the part of fsl_ssi_set_bclk() function which is called after > fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;). > > Before the aforementioned check we do have: > > if (ssi_private->bitclk_freq) > freq = ssi_private->bitclk_freq; > else > freq = params_channels(hw_params) * 32 * > params_rate(hw_params); > > > Which assigns freq = bitclk_freq (66 MHz) > > And then we break on this particular check: > > 66MHz * 5 > 66 MHz. > > > > The culprit IMHO is the ssi_private->bitclk_freq = freq; in the > fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock > (ssi_private->clk), not the bit clock (BCLK). > > > This patch just quits early if it detects change, which don't need to be > done. Thanks for the clarification. Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote: > >/* > >* Hardware limitation: The bclk rate must be > >* never greater than 1/5 IPG clock rate > >*/ > >if (freq * 5 > clk_get_rate(ssi_private->clk)) { > >dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); > >return -EINVAL; > >} > > > > Unfortunately not. > > This is the part of fsl_ssi_set_bclk() function which is called after > fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;). > > Before the aforementioned check we do have: > > if (ssi_private->bitclk_freq) > freq = ssi_private->bitclk_freq; > else > freq = params_channels(hw_params) * 32 * params_rate(hw_params); > > > Which assigns freq = bitclk_freq (66 MHz) > [...] > And then we break on this particular check: > 66MHz * 5 > 66 MHz. [...] Does the check fail and return an error here, or not? > The culprit IMHO is the ssi_private->bitclk_freq = freq; in the > fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock > (ssi_private->clk), not the bit clock (BCLK). No. We should not set the IP block clock. That's from IPG bus on certain IMX SoCs. Setting it might change IPG bus clocks which might break the system. And apparently, we shouldn't set bitclk to 66MHz either. Can you help to find where this 66MHz comes from? > This patch just quits early if it detects change, which don't need to be > done. I kinda see your point is to error out before hw_params(). So I feel it would be better to have a similar 5-times-check in the set_sysclk() too, if it's really necessary. Btw, I don't see simple card calling set_sysclk() function in any earlier stage than hw_params(). I am wondering how did you encounter the problem? Thanks Nicolin
On 09/05/2017 07:20 AM, Nicolin Chen wrote: > On Sun, Sep 03, 2017 at 04:40:21PM +0200, Łukasz Majewski wrote: >>> /* >>> * Hardware limitation: The bclk rate must be >>> * never greater than 1/5 IPG clock rate >>> */ >>> if (freq * 5 > clk_get_rate(ssi_private->clk)) { >>> dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); >>> return -EINVAL; >>> } >>> >> >> Unfortunately not. >> >> This is the part of fsl_ssi_set_bclk() function which is called after >> fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;). >> >> Before the aforementioned check we do have: >> >> if (ssi_private->bitclk_freq) >> freq = ssi_private->bitclk_freq; >> else >> freq = params_channels(hw_params) * 32 * params_rate(hw_params); >> >> >> Which assigns freq = bitclk_freq (66 MHz) >> > [...] >> And then we break on this particular check: >> 66MHz * 5 > 66 MHz. > [...] > > Does the check fail and return an error here, or not? Yes, this check fails and return error (-EINVAL). > >> The culprit IMHO is the ssi_private->bitclk_freq = freq; in the >> fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock >> (ssi_private->clk), not the bit clock (BCLK). > > No. We should not set the IP block clock. That's from IPG bus > on certain IMX SoCs. Setting it might change IPG bus clocks > which might break the system. > > And apparently, we shouldn't set bitclk to 66MHz either. Can > you help to find where this 66MHz comes from? OK. The fsi_ssi.c file defines: ops->set_sysclk() routine: .set_sysclk = fsl_ssi_set_dai_sysclk, This routine is called in: int snd_soc_dai_set_sysclk() @ soc-core.c which is called in two places for my setup: 1. asoc_simple_card_hw_params() @ simple-card.c and 2. int asoc_simple_card_init_dai() @ simple-card-utils.c In this function (point 2.) the simple_dai->sysclk is set and: snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) which sets frequency to 66 MHz [*]. The asoc_simple_card_init_dai() is called in asoc_simple_card_dai_init() @ simple-card.c which is assigned to dai_link->init dai_link->init = asoc_simple_card_dai_init; @ simple_card.c And the sysclk itself is defined at: ------------------------------------- dai_props->codec_dai->sysclk, which is used at: asoc_simple_card_startup(), asoc_simple_card_shutdown() and others functions at simple-card.c It is setup at: asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: #define asoc_simple_card_parse_clk_cpu() And the problem is: ------------------- At the asoc_simple_card_parse_clk() we finally go to dts node: /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C which has clock from I2C (66 MHz). (So the [*] hack may help here, since it is checked before checking the i2c node). Note: [*] - I could workaround this problem by setting: system-clock-frequency = <0> in dailink_master: cpu { sound-dai = <&ssi2>; }; but this is IMHO even worse hack.... than this patch. > >> This patch just quits early if it detects change, which don't need to be >> done. > > I kinda see your point is to error out before hw_params(). So > I feel it would be better to have a similar 5-times-check in > the set_sysclk() too, if it's really necessary. > > Btw, I don't see simple card calling set_sysclk() function in > any earlier stage than hw_params(). I am wondering how did you > encounter the problem? > > Thanks > Nicolin >
On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote: > >And apparently, we shouldn't set bitclk to 66MHz either. Can > >you help to find where this 66MHz comes from? > 2. int asoc_simple_card_init_dai() @ simple-card-utils.c Oh, I just searched in the simple-card.c but missed this file. > In this function (point 2.) the > simple_dai->sysclk is set and: > snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) > which sets frequency to 66 MHz [*]. > > The asoc_simple_card_init_dai() is called in > asoc_simple_card_dai_init() @ simple-card.c > which is assigned to dai_link->init > dai_link->init = asoc_simple_card_dai_init; @ simple_card.c > > And the sysclk itself is defined at: > ------------------------------------- > dai_props->codec_dai->sysclk, which is used at:a Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking about SSI? > asoc_simple_card_startup(), asoc_simple_card_shutdown() and others > functions at simple-card.c > It is setup at: > asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: > #define asoc_simple_card_parse_clk_cpu() > And the problem is: > ------------------- > > At the > asoc_simple_card_parse_clk() > we finally go to dts node: > /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C This tfa9879 should be the CODEC right? > which has clock from I2C (66 MHz). You mean I2C scl or I2S sclk? ----------------------------------------------------------------- But anyway, I feel very confused here as you have 66MHz clock rate (regardless of it purpose) for a codec dai but it's been passed to a cpu dai (SSI). > [*] - I could workaround this problem by setting: > > system-clock-frequency = <0> in > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > > but this is IMHO even worse hack.... than this patch. I haven't used simple-card for a while so I forgot how to define its DT bindings specifically. But you should assign ssi2 as the CPU dai and assign tfa9879 as a CODEC dai.
Hi Lukasz, On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski <lukma@denx.de> wrote: > Note: > [*] - I could workaround this problem by setting: > > system-clock-frequency = <0> in > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > Which mx6 type are you using? Can you post the complete audio related bindings of your dts? Still trying to understand why we do not see this error on other imx6 boards.
Hi Nicolin, > On Tue, Sep 05, 2017 at 10:35:34AM +0200, Łukasz Majewski wrote: > >>> And apparently, we shouldn't set bitclk to 66MHz either. Can >>> you help to find where this 66MHz comes from? > >> 2. int asoc_simple_card_init_dai() @ simple-card-utils.c > > Oh, I just searched in the simple-card.c but missed this file. > >> In this function (point 2.) the >> simple_dai->sysclk is set and: >> snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0) >> which sets frequency to 66 MHz [*]. >> >> The asoc_simple_card_init_dai() is called in >> asoc_simple_card_dai_init() @ simple-card.c >> which is assigned to dai_link->init >> dai_link->init = asoc_simple_card_dai_init; @ simple_card.c >> >> And the sysclk itself is defined at: >> ------------------------------------- >> dai_props->codec_dai->sysclk, which is used at:a > > Why codec_dai? Why not dai_props->cpu_dai->sysclk since we are talking > about SSI? This is how the simple-card (simple-sound-card) is written. > >> asoc_simple_card_startup(), asoc_simple_card_shutdown() and others >> functions at simple-card.c >> It is setup at: >> asoc_simple_card_parse_clk() @ simple-card-utils.c from macro: >> #define asoc_simple_card_parse_clk_cpu() >> And the problem is: >> ------------------- >> >> At the >> asoc_simple_card_parse_clk() >> we finally go to dts node: >> /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C > > This tfa9879 should be the CODEC right? Yes. The tfa9879 is a codec (very simple -> I2S + I2C, mono). They key point here is the asoc_simple_card_parse_clk() function from simple-card-utils.c Please look how the clock is assigned; It first checks for cpu clock, then for "system-clock-frequency" DTS node and _finally_ looks for another "child" clock [1], which is the codec attached to I2C. And from there it takes the 66 MHz CLK: /soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C > >> which has clock from I2C (66 MHz). > > You mean I2C scl or I2S sclk? I2C scl. > > ----------------------------------------------------------------- > > But anyway, I feel very confused here as you have 66MHz clock rate > (regardless of it purpose) for a codec dai but it's been passed to > a cpu dai (SSI). Please look into asoc_simple_card_parse_clk(). My DTS [1] (it is different than other in-tree supported codecs - at least I did not find similar setup in DTSes): sound { compatible = "simple-audio-card"; label = "tfa9879-mono"; simple-audio-card,dai-link { /* DAC */ format = "i2s"; bitclock-master = <&dailink_master>; frame-master = <&dailink_master>; dailink_master: cpu { sound-dai = <&ssi2>; }; codec { sound-dai = <&codec>; }; }; }; &i2c1 { clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c1>; status = "okay"; codec: tfa9879@6C { #sound-dai-cells = <0>; compatible = "tfa9879"; reg = <0x6C>; }; }; &ssi2 { fsl,mode = "i2s-master"; status = "okay"; }; the ssi2 node is defined in imx6qdl.dtsi file (no changes). The SOC is IMX6Q. The TFA9879 is a slave for I2S transmission. > >> [*] - I could workaround this problem by setting: >> >> system-clock-frequency = <0> in >> >> dailink_master: cpu { >> sound-dai = <&ssi2>; >> }; >> >> but this is IMHO even worse hack.... than this patch. > > I haven't used simple-card for a while so I forgot how to define > its DT bindings specifically. But you should assign ssi2 as the > CPU dai and assign tfa9879 as a CODEC dai. >
Hi Fabio, > Hi Lukasz, > > On Tue, Sep 5, 2017 at 5:35 AM, Łukasz Majewski <lukma@denx.de> wrote: > >> Note: >> [*] - I could workaround this problem by setting: >> >> system-clock-frequency = <0> in >> >> dailink_master: cpu { >> sound-dai = <&ssi2>; >> }; >> > > Which mx6 type are you using? Can you post the complete audio related > bindings of your dts? > > Still trying to understand why we do not see this error on other imx6 boards. > I've sent my DTS in the other e-mail.
On Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote: > They key point here is the asoc_simple_card_parse_clk() function > from simple-card-utils.c > > Please look how the clock is assigned; It first checks for cpu > clock, then for "system-clock-frequency" DTS node and _finally_ > looks for another "child" clock [1], which is the codec attached to > I2C. Here is the routine that I understood from the code: 1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] dai_link->cpu_of_node, // node ssi2 [2] cpu_dai, dai_link->cpu_dai_name); ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2] 2) asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai); => asoc_simple_card_parse_clk(dev, codec, // codec node in sound{} [3] dai_link->codec_of_node,// node tfa9879 [4] codec_dai, dai_link->codec_dai_name); ==> 2.1) devm_get_clk_from_child(dev, node, NULL); // [3] ==> 2.2) of_property_read_u32(node, "system-clock-frequency", &val)// [3] ==> 2.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [4] For the cpu routine, it first checks for clock property under cpu node of simple-card, then for "system-clock-frequency" in the cpu node of simple-card, and finally looks for clock property in ssi2 node. For codec routine, it first checks for clock property under codec node of simple-card, then for "system-clock-frequency" in codec node of simple-card, and finally looks for clock property in the tfa9879 node. The cpu and codec are totally separate, aren't they? And I don't think it's right if not. > >>which has clock from I2C (66 MHz). > > > >You mean I2C scl or I2S sclk? > > I2C scl. A couple of things here..... 1) I don't think I2C scl can run at 66MHz...The 66MHz is probably the ipg clock of I2C controller. 2) Even if the scl does run at 66MHz, there is no point in passing the clock of I2C scl into an I2S dai-link. So I feel something must be wrong. I suggest you to add some printk to check the names of those nodes and x_of_node in the simple card driver. > My DTS [1] (it is different than other in-tree supported codecs - at > least I did not find similar setup in DTSes): > > sound { > compatible = "simple-audio-card"; > label = "tfa9879-mono"; > > simple-audio-card,dai-link { > /* DAC */ > format = "i2s"; > bitclock-master = <&dailink_master>; > frame-master = <&dailink_master>; > > dailink_master: cpu { > sound-dai = <&ssi2>; > }; > codec { > sound-dai = <&codec>; > }; > }; I remember there is another style for a single dai-link. Could you please try that one? There is an example in: Documentation/devicetree/bindings/sound/simple-card.txt
Hi Nicolin, > On Tue, Sep 05, 2017 at 11:13:40PM +0200, Łukasz Majewski wrote: > >> They key point here is the asoc_simple_card_parse_clk() function >> from simple-card-utils.c >> >> Please look how the clock is assigned; It first checks for cpu >> clock, then for "system-clock-frequency" DTS node and _finally_ >> looks for another "child" clock [1], which is the codec attached to >> I2C. > > Here is the routine that I understood from the code: > 1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); > => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] > dai_link->cpu_of_node, // node ssi2 [2] > cpu_dai, dai_link->cpu_dai_name); > ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] > ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] > ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2] > > 2) asoc_simple_card_parse_clk_codec(dev, codec, dai_link, codec_dai); > => asoc_simple_card_parse_clk(dev, codec, // codec node in sound{} [3] > dai_link->codec_of_node,// node tfa9879 [4] > codec_dai, dai_link->codec_dai_name); > ==> 2.1) devm_get_clk_from_child(dev, node, NULL); // [3] > ==> 2.2) of_property_read_u32(node, "system-clock-frequency", &val)// [3] > ==> 2.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [4] > > For the cpu routine, it first checks for clock property under cpu > node of simple-card, then for "system-clock-frequency" in the cpu > node of simple-card, and finally looks for clock property in ssi2 > node. I've double checked this and the simple_dai->sysclk is assigned in the asoc_simple_card_parse_clk() -----> dev: sound -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 -----> Clk asignment And this clock is taken from this node. It looks like ipg clock for ssi... > > For codec routine, it first checks for clock property under codec > node of simple-card, then for "system-clock-frequency" in codec > node of simple-card, and finally looks for clock property in the > tfa9879 node. > > The cpu and codec are totally separate, aren't they? And I don't > think it's right if not. Yes, they should be separated. > >>>> which has clock from I2C (66 MHz). >>> >>> You mean I2C scl or I2S sclk? >> >> I2C scl. > > A couple of things here..... > 1) I don't think I2C scl can run at 66MHz...The 66MHz is probably > the ipg clock of I2C controller. I agree. I2C scl runs with 400 kHz (as recommended by I2C high speed spec). And considering above, the 66 MHz clock seems to be from ssi IP block. > 2) Even if the scl does run at 66MHz, there is no point in passing > the clock of I2C scl into an I2S dai-link. Yes. > > So I feel something must be wrong. I suggest you to add some printk > to check the names of those nodes and x_of_node in the simple card > driver. The problem is with the "lack" of clock nodes/properties at dailink_master: cpu { sound-dai = <&ssi2>; clock = <&SSSS>; system-clock-frequency = <XXXX>; }; and as a result we take some "default" clock. > >> My DTS [1] (it is different than other in-tree supported codecs - at >> least I did not find similar setup in DTSes): >> >> sound { >> compatible = "simple-audio-card"; >> label = "tfa9879-mono"; >> >> simple-audio-card,dai-link { >> /* DAC */ >> format = "i2s"; >> bitclock-master = <&dailink_master>; >> frame-master = <&dailink_master>; >> >> dailink_master: cpu { >> sound-dai = <&ssi2>; >> }; >> codec { >> sound-dai = <&codec>; >> }; >> }; > > I remember there is another style for a single dai-link. Could you > please try that one? There is an example in: > Documentation/devicetree/bindings/sound/simple-card.txt Yes, the simple-audio-card,cpu simple-audio-card,format, .... etc. I've omitted it since in the code it is regarded as "old": (simple-card.c line 384). > I think that the proper solution would be to add check for: freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to make the simple-audo-card driver happy (and not introducing regressions).
On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote: > >Here is the routine that I understood from the code: > >1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); > > => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] > > dai_link->cpu_of_node, // node ssi2 [2] > > cpu_dai, dai_link->cpu_dai_name); > > ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] > > ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] > > ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2] > >For the cpu routine, it first checks for clock property under cpu > >node of simple-card, then for "system-clock-frequency" in the cpu > >node of simple-card, and finally looks for clock property in ssi2 > >node. > -----> dev: sound > -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 > -----> Clk asignment > > And this clock is taken from this node. It looks like ipg clock for ssi... This makes sense now. The devm_get_clk_from_child() in 1.3) fetched the first clock of ssi2 -- ipg clock. > The problem is with the "lack" of clock nodes/properties at > > dailink_master: cpu { > sound-dai = <&ssi2>; > clock = <&SSSS>; > system-clock-frequency = <XXXX>; > }; This is the right solution based on current simple-card driver. For SSI (having two clocks), you have to specify the baud clock in the cpu node like that. I believe this is what the simple-card designer expected users to do since the cpu node is the first place that the driver tries to look at. > I think that the proper solution would be to add check for: > > freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to > make the simple-audo-card driver happy (and not introducing > regressions). As I said in the first place, adding another check in set_sysclk() is not that essential but seems to be plausible to me. So I am okay if you really want to have that.
Hi Nicolin, > On Wed, Sep 06, 2017 at 11:22:48AM +0200, Łukasz Majewski wrote: > >>> Here is the routine that I understood from the code: >>> 1) asoc_simple_card_parse_clk_cpu(dev, cpu, dai_link, cpu_dai); >>> => asoc_simple_card_parse_clk(dev, cpu, // cpu node in sound{} [1] >>> dai_link->cpu_of_node, // node ssi2 [2] >>> cpu_dai, dai_link->cpu_dai_name); >>> ==> 1.1) devm_get_clk_from_child(dev, node, NULL); // [1] >>> ==> 1.2) of_property_read_u32(node, "system-clock-frequency", &val)// [1] >>> ==> 1.3) devm_get_clk_from_child(dev, dai_of_node, NULL); // [2] > >>> For the cpu routine, it first checks for clock property under cpu >>> node of simple-card, then for "system-clock-frequency" in the cpu >>> node of simple-card, and finally looks for clock property in ssi2 >>> node. > >> -----> dev: sound >> -----> clk node: /soc/aips-bus@02000000/spba-bus@02000000/ssi@0202c000 >> -----> Clk asignment >> >> And this clock is taken from this node. It looks like ipg clock for ssi... > > This makes sense now. The devm_get_clk_from_child() in 1.3) fetched > the first clock of ssi2 -- ipg clock. Yes, exactly (for SSI2) [1]: clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, <&clks IMX6QDL_CLK_SSI2>; clock-names = "ipg", "baud"; > >> The problem is with the "lack" of clock nodes/properties at >> >> dailink_master: cpu { >> sound-dai = <&ssi2>; >> clock = <&SSSS>; If possible I do prefer a solution, which uses only DTS. Side question - how to refer to baud clock from [1]? >> system-clock-frequency = <XXXX>; >> }; > > This is the right solution based on current simple-card driver. For > SSI (having two clocks), you have to specify the baud clock in the > cpu node like that. I believe this is what the simple-card designer > expected users to do since the cpu node is the first place that the > driver tries to look at. I will give a shoot the option with adding the ipg clock. > >> I think that the proper solution would be to add check for: >> >> freq < sysclk/5 in fsl_ssi_set_dai_sysclk() and return -ENOTSUPP to >> make the simple-audo-card driver happy (and not introducing >> regressions). > > As I said in the first place, adding another check in set_sysclk() > is not that essential but seems to be plausible to me. So I am okay > if you really want to have that. > Thanks for help.
On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote: > clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, > <&clks IMX6QDL_CLK_SSI2>; > clock-names = "ipg", "baud"; > >> dailink_master: cpu { > >> sound-dai = <&ssi2>; > >> clock = <&SSSS>; > > If possible I do prefer a solution, which uses only DTS. > Side question - how to refer to baud clock from [1]? Just add a property to this cpu node like: clock = <&clks IMX6QDL_CLK_SSI2>; > >> system-clock-frequency = <XXXX>; This would not be necessary unless you want to specify a clock rate so as to override the clock rate configuration in hw_params(). > >This is the right solution based on current simple-card driver. For > >SSI (having two clocks), you have to specify the baud clock in the > >cpu node like that. I believe this is what the simple-card designer > >expected users to do since the cpu node is the first place that the > >driver tries to look at. > > I will give a shoot the option with adding the ipg clock. No, not ipg clock. You should use the second clock -- baud clock.
On 09/06/2017 09:47 PM, Nicolin Chen wrote: > On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote: > >> clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, >> <&clks IMX6QDL_CLK_SSI2>; >> clock-names = "ipg", "baud"; > >>>> dailink_master: cpu { >>>> sound-dai = <&ssi2>; >>>> clock = <&SSSS>; >> >> If possible I do prefer a solution, which uses only DTS. >> Side question - how to refer to baud clock from [1]? > > Just add a property to this cpu node like: > clock = <&clks IMX6QDL_CLK_SSI2>; Ok. > >>>> system-clock-frequency = <XXXX>; > > This would not be necessary unless you want to specify a clock rate > so as to override the clock rate configuration in hw_params(). Ok. > >>> This is the right solution based on current simple-card driver. For >>> SSI (having two clocks), you have to specify the baud clock in the >>> cpu node like that. I believe this is what the simple-card designer >>> expected users to do since the cpu node is the first place that the >>> driver tries to look at. >> >> I will give a shoot the option with adding the ipg clock. > > No, not ipg clock. You should use the second clock -- baud clock. Yes. Correct - the baud clock. >
Hi Nicolin, > On Wed, Sep 06, 2017 at 08:35:50PM +0200, Łukasz Majewski wrote: > >> clocks = <&clks IMX6QDL_CLK_SSI2_IPG>, >> <&clks IMX6QDL_CLK_SSI2>; >> clock-names = "ipg", "baud"; > >>>> dailink_master: cpu { >>>> sound-dai = <&ssi2>; >>>> clock = <&SSSS>; >> >> If possible I do prefer a solution, which uses only DTS. >> Side question - how to refer to baud clock from [1]? > > Just add a property to this cpu node like: > clock = <&clks IMX6QDL_CLK_SSI2>; This doesn't solve the issue: root@display5:~# speaker-test speaker-test 1.1.3 Playback device is default Stream parameters are 48000Hz, S16_LE, 1 channels Using 16 octaves of pink noise Rate set to 48000Hz (requested 48fsl-ssi-dai 202c000.ssi: bitclk > ipgclk/5 000Hz) Buffer size range from 64fsl-ssi-dai 202c000.ssi: ASoC: can't set 202c000.ssi hw params: -22 to 65536 Period size range from 32 to 8191 Using max buffer size 65536 Periods = 4 Unable to set hw params for playback: Invalid argument Setting of hwparams failed: Invalid argument > >>>> system-clock-frequency = <XXXX>; > > This would not be necessary unless you want to specify a clock rate > so as to override the clock rate configuration in hw_params(). > >>> This is the right solution based on current simple-card driver. For >>> SSI (having two clocks), you have to specify the baud clock in the >>> cpu node like that. I believe this is what the simple-card designer >>> expected users to do since the cpu node is the first place that the >>> driver tries to look at. >> >> I will give a shoot the option with adding the ipg clock. > > No, not ipg clock. You should use the second clock -- baud clock. >
On Fri, Sep 08, 2017 at 01:10:12AM +0200, Łukasz Majewski wrote: > >Just add a property to this cpu node like: > > clock = <&clks IMX6QDL_CLK_SSI2>; > > This doesn't solve the issue: I have a patch locally that should be able to solve your problem. But I need to first verify on my board tonight and will send it later (will put you in the TO/CC list).
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 173cb84..1186fa9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -809,6 +809,8 @@ static int fsl_ssi_set_dai_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, unsigned int freq, int dir) { struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai); + if (clk_get_rate(ssi_private->clk) == freq) + return 0; ssi_private->bitclk_freq = freq;
[Sorry for the top-posting] The driver currently has: /* * Hardware limitation: The bclk rate must be * never greater than 1/5 IPG clock rate */ if (freq * 5 > clk_get_rate(ssi_private->clk)) { dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n"); return -EINVAL; } Isn't this properly taking care of the clock restriction?