| Message ID | 1504436701-20700-1-git-send-email-lukma@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/05/2017 07:06 AM, Nicolin Chen wrote: > On Sun, Sep 03, 2017 at 01:05:01PM +0200, Lukasz Majewski wrote: >> 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]. > > I think a bigger question here is why the routine sets BCLK to 66MHz. Yes, exactly. In my case the bclk is set to ipg clock, which is the SSI IP block clock (ipg). > >> 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]. > > I don't feel it's quite comprehensive...what if it's being set to 67MHz. I think that this clock is not changing for the SoC. It should be 66 MHz fixed. > > Thanks > Nicolin > >> 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 Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote: > >>The last call is changing the bit clock (BCLK) frequency to SSI's IP > >>block clock (ipg = 66 MHz) [1]. > > > >I think a bigger question here is why the routine sets BCLK to 66MHz. > > Yes, exactly. > > In my case the bclk is set to ipg clock, which is the SSI IP block clock > (ipg). Can you elaborate why you set ipg clock as bclk? I don't remember SSI could derive bitclock from ipg clock. > >>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]. > > > >I don't feel it's quite comprehensive...what if it's being set to 67MHz. > > I think that this clock is not changing for the SoC. It should be 66 MHz > fixed. What I mean is that we cannot just look at this SoC. Today is 66MHz for this SoC. Tomorrow could be 133MHz for another one. We should put a check that none of these shall pass -- the 1/5 limit.
On 09/05/2017 09:52 AM, Nicolin Chen wrote: > On Tue, Sep 05, 2017 at 09:37:43AM +0200, Łukasz Majewski wrote: > >>>> The last call is changing the bit clock (BCLK) frequency to SSI's IP >>>> block clock (ipg = 66 MHz) [1]. >>> >>> I think a bigger question here is why the routine sets BCLK to 66MHz. >> >> Yes, exactly. >> >> In my case the bclk is set to ipg clock, which is the SSI IP block clock >> (ipg). > > Can you elaborate why you set ipg clock as bclk? I don't remember SSI could > derive bitclock from ipg clock. Just to be clear: What clock shall be set with: struct snd_soc_dai_ops { int (*set_sysclk)(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir); } callback? The SSI IP block or BCLK ? > >>>> 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]. >>> >>> I don't feel it's quite comprehensive...what if it's being set to 67MHz. >> >> I think that this clock is not changing for the SoC. It should be 66 MHz >> fixed. > > What I mean is that we cannot just look at this SoC. Today is 66MHz for this > SoC. Tomorrow could be 133MHz for another one. We should put a check that none > of these shall pass -- the 1/5 limit. > Ok. Good point.
On Tue, Sep 05, 2017 at 10:19:05AM +0200, Łukasz Majewski wrote: > On 09/05/2017 09:52 AM, Nicolin Chen wrote: > > Can you elaborate why you set ipg clock as bclk? I don't remember SSI could > > derive bitclock from ipg clock. > Just to be clear: > What clock shall be set with: > struct snd_soc_dai_ops { > int (*set_sysclk)(struct snd_soc_dai *dai, > int clk_id, unsigned int freq, int dir); > } > callback? > The SSI IP block or BCLK ? Not the BCLK, probably the IP clock but perhaps nothing. The bclk is usually derived from the sample rate and width, the system clock is the clock rate going into the device. It doesn't *have* to be configured (particuarly if it's discoverable by the IP and managable via the clock API).
On Tue, Sep 05, 2017 at 04:15:50PM +0100, Mark Brown wrote: > > Just to be clear: > > > What clock shall be set with: > > > struct snd_soc_dai_ops { > > int (*set_sysclk)(struct snd_soc_dai *dai, > > int clk_id, unsigned int freq, int dir); > > } > > > callback? > > > The SSI IP block or BCLK ? > > Not the BCLK, probably the IP clock but perhaps nothing. The bclk is > usually derived from the sample rate and width, the system clock is the > clock rate going into the device. It doesn't *have* to be configured > (particuarly if it's discoverable by the IP and managable via the clock > API). Hmm...to clarify the things, some background here: SSI has mainly two different clock inputs internally (not external pads such as bclk or lrclk): IP block clock (ipg_clk in Reference Manual) and sys clock (ccm_ssi_clk in Reference Manual). According to RM, ccm_ssi_clk: "module/system clock for bit clock generation". The ipg clock is merely used to access registers, and has nothing (directly) to do with external clock outputs. The driver shall not change the ipg clock as the system ipg clock (its parent clock) might be messed and even system time would get weird -- happened once when the fsl_spdif driver used to call clk_set_rate() on its ipg clock. Although the clock controller should have some kind of protection in my opinion, we just avoid IP clock rate change in all audio drivers as well. On the other hand, the sys clock (baudclk in the driver) should be configured whenever it's related to external clock outputs. When I implemented this set_sysclk() for fsl_ssi.c, I used it to set this sys clock (baudclk) by a machine driver, in order to set bit clock. Then someone patched the driver by moving all the code to set_bclk() to make machine drivers simpler. Now the set_sysclk() is remained to give machine drivers a chance to override clock configurations in the hw_params(). This could be used in TDM or some other special cases (It could also have a purpose for backwards compatibility). So here, we should set baudclk (BCLK generator).
On Tue, Sep 05, 2017 at 10:45:29AM -0700, Nicolin Chen wrote: > The ipg clock is merely used to access registers, and has nothing > (directly) to do with external clock outputs. The driver shall not > change the ipg clock as the system ipg clock (its parent clock) > might be messed and even system time would get weird -- happened > once when the fsl_spdif driver used to call clk_set_rate() on its > ipg clock. Although the clock controller should have some kind of > protection in my opinion, we just avoid IP clock rate change in all > audio drivers as well. Yes, the clock API needs constraints code. > On the other hand, the sys clock (baudclk in the driver) should be > configured whenever it's related to external clock outputs. When I > implemented this set_sysclk() for fsl_ssi.c, I used it to set this > sys clock (baudclk) by a machine driver, in order to set bit clock. > Then someone patched the driver by moving all the code to set_bclk() > to make machine drivers simpler. Now the set_sysclk() is remained > to give machine drivers a chance to override clock configurations > in the hw_params(). This could be used in TDM or some other special > cases (It could also have a purpose for backwards compatibility). > So here, we should set baudclk (BCLK generator). No, that's just going to cause confusion - if all the other drivers are using set_sysclk() to set an input clock rate to the IP rather than an output clock but your driver does something else then sooner or later someone will run into trouble with that.
On Thu, Sep 07, 2017 at 02:44:11PM +0100, Mark Brown wrote: > > On the other hand, the sys clock (baudclk in the driver) should be > > configured whenever it's related to external clock outputs. When I > > implemented this set_sysclk() for fsl_ssi.c, I used it to set this > > sys clock (baudclk) by a machine driver, in order to set bit clock. > > Then someone patched the driver by moving all the code to set_bclk() > > to make machine drivers simpler. Now the set_sysclk() is remained > > to give machine drivers a chance to override clock configurations > > in the hw_params(). This could be used in TDM or some other special > > cases (It could also have a purpose for backwards compatibility). > > > So here, we should set baudclk (BCLK generator). > > No, that's just going to cause confusion - if all the other drivers are > using set_sysclk() to set an input clock rate to the IP rather than an > output clock but your driver does something else then sooner or later > someone will run into trouble with that. I admit I had that concern. Probably I should have deprecated this set_sysclk(). I will try to patch it and hw_params() accordingly.
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;
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(+)