Re: [alsa-devel] [PATCH] ASoC: sgtl5000: add support for a fixed sysclk frequency
[Sorry for the top-posting. I did not see your message in the mailing list] Hi Jakob, arch/arm/boot/dts/imx51-babbage.dts has SYS_MCLK generated by an external fixed oscillator and it works fine. Do you really need this new property? ________________________________ From: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> Sent: Friday, October 6, 2017 5:34:44 AM To: linux-kernel@vger.kernel.org Cc: Jakob Unterwurzacher; Klaus Goger; Liam Girdwood; Mark Brown; Rob Herring; Mark Rutland; Jaroslav Kysela; Takashi Iwai; Richard Leitner; Fabio Estevam; Bhumika Goyal; alsa-devel@alsa-project.org; devicetree@vger.kernel.org Subject: [PATCH] ASoC: sgtl5000: add support for a fixed sysclk frequency The system clock, also called SYS_MCLK in the SGTL5000 datasheet, is usually generated by the I2S master with a frequency proportional to the sample rate. However, this is not always the case. Compute modules following the Qseven specification, for example, do not even have a pin for I2S SYS_MCLK on their board-to-board connector. Only I2S_WS, I2S_RST# I2S_CLK, I2S_SDI and I2S_SDO are specified. Instead, SYS_MCLK is usually generated by an external fixed oscillator. Add support for this scenario, enabled by the new "sysclk-fixed" devicetree property. Tested on a "Puma" RK3399-Q7 compute module with a "Haikou" baseboard that has a fixed 24.576 MHz oscillator connected to the codec's SYS_MCLK. Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> --- Documentation/devicetree/bindings/sound/sgtl5000.txt | 5 +++++ sound/soc/codecs/sgtl5000.c | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/Documentation/devicetree/bindings/sound/sgtl5000.txt b/Documentation/devicetree/bindings/sound/sgtl5000.txt index 7a73a9d62015..0d0dd58b2d4a 100644 --- a/Documentation/devicetree/bindings/sound/sgtl5000.txt +++ b/Documentation/devicetree/bindings/sound/sgtl5000.txt @@ -35,6 +35,11 @@ VDDIO 1.8V 2.5V 3.3V 2 = 3.33 mA 5.74 mA 8.03 mA 3 = 4.99 mA 8.61 mA 12.05 mA +- sysclk-fixed : If this property exists, the sysclk is considered to be + generated by a fixed-frequency oscillator. The frequency value is read + from the clock defined in the "clocks" property. set_sysclk requests + are ignored. + Example: codec: sgtl5000@0a { diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index f2bb4feba3b6..be3aea89371c 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -133,6 +133,7 @@ struct sgtl5000_priv { u8 micbias_resistor; u8 micbias_voltage; u8 lrclk_strength; + u8 sysclk_fixed; /* sysclk rate cannot be changed */ }; /* @@ -613,6 +614,10 @@ static int sgtl5000_set_dai_sysclk(struct snd_soc_dai *codec_dai, switch (clk_id) { case SGTL5000_SYSCLK: + if (sgtl5000->sysclk_fixed) { + dev_dbg(codec->dev, "sysclk-fixed: ignoring set_sysclk request\n"); + return 0; + } sgtl5000->sysclk = freq; break; default: @@ -1444,6 +1449,13 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, } else { sgtl5000->micbias_voltage = 0; } + if (of_property_read_bool(np, "sysclk-fixed")) { + sgtl5000->sysclk = clk_get_rate(sgtl5000->mclk); + sgtl5000->sysclk_fixed = 1; + dev_dbg(&client->dev, + "sysclk-fixed: setting sysclk to fixed value %d\n", + sgtl5000->sysclk); + } } sgtl5000->lrclk_strength = I2S_LRCLK_STRENGTH_LOW; -- 2.11.0
Hi Fabio, that’s interesting. Looks like the fsl,imx-audio-sgtl5000 sound card driver that is used on the imx51-babbage does not try to adjust MCLK in response to the playback sample rate. The simple-audio-card driver does that (it calls snd_soc_dai_set_sysclk whenever playback starts). I think we can do without the new dts property when we fix sgtl5000_set_dai_sysclk to actually call clk_set_rate on the MCLK instead of blindly setting any requested value. On a fixed clock, clk_set_rate is a no-op if the requested rate matches the fixed rate and errors out otherwise. I think this would be the more correct fix, but it carries the risk of breaking existing boards that rely on the current behaviour. What do you think? Best regards, Jakob
On 16. Oct 2017, at 01:09, Fabio Estevam <fabio.estevam@nxp.com> wrote:
[Sorry for the top-posting. I did not see your message in the mailing list]
Hi Jakob,
arch/arm/boot/dts/imx51-babbage.dts has SYS_MCLK generated by an external fixed oscillator and it works fine.
Do you really need this new property? From: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> Sent: Friday, October 6, 2017 5:34:44 AM To: linux-kernel@vger.kernel.org Cc: Jakob Unterwurzacher; Klaus Goger; Liam Girdwood; Mark Brown; Rob Herring; Mark Rutland; Jaroslav Kysela; Takashi Iwai; Richard Leitner; Fabio Estevam; Bhumika Goyal; alsa-devel@alsa-project.org; devicetree@vger.kernel.org Subject: [PATCH] ASoC: sgtl5000: add support for a fixed sysclk frequency
The system clock, also called SYS_MCLK in the SGTL5000 datasheet, is usually generated by the I2S master with a frequency proportional to the sample rate.
However, this is not always the case. Compute modules following the Qseven specification, for example, do not even have a pin for I2S SYS_MCLK on their board-to-board connector. Only I2S_WS, I2S_RST# I2S_CLK, I2S_SDI and I2S_SDO are specified.
Instead, SYS_MCLK is usually generated by an external fixed oscillator.
Add support for this scenario, enabled by the new "sysclk-fixed" devicetree property.
Tested on a "Puma" RK3399-Q7 compute module with a "Haikou" baseboard that has a fixed 24.576 MHz oscillator connected to the codec's SYS_MCLK.
Signed-off-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> --- Documentation/devicetree/bindings/sound/sgtl5000.txt | 5 +++++ sound/soc/codecs/sgtl5000.c | 12 ++++++++++++ 2 files changed, 17 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/sgtl5000.txt b/Documentation/devicetree/bindings/sound/sgtl5000.txt index 7a73a9d62015..0d0dd58b2d4a 100644 --- a/Documentation/devicetree/bindings/sound/sgtl5000.txt +++ b/Documentation/devicetree/bindings/sound/sgtl5000.txt @@ -35,6 +35,11 @@ VDDIO 1.8V 2.5V 3.3V 2 = 3.33 mA 5.74 mA 8.03 mA 3 = 4.99 mA 8.61 mA 12.05 mA
+- sysclk-fixed : If this property exists, the sysclk is considered to be + generated by a fixed-frequency oscillator. The frequency value is read + from the clock defined in the "clocks" property. set_sysclk requests + are ignored. + Example:
codec: sgtl5000@0a { diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index f2bb4feba3b6..be3aea89371c 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -133,6 +133,7 @@ struct sgtl5000_priv { u8 micbias_resistor; u8 micbias_voltage; u8 lrclk_strength; + u8 sysclk_fixed; /* sysclk rate cannot be changed */ };
/* @@ -613,6 +614,10 @@ static int sgtl5000_set_dai_sysclk(struct snd_soc_dai *codec_dai,
switch (clk_id) { case SGTL5000_SYSCLK: + if (sgtl5000->sysclk_fixed) { + dev_dbg(codec->dev, "sysclk-fixed: ignoring set_sysclk request\n"); + return 0; + } sgtl5000->sysclk = freq; break; default: @@ -1444,6 +1449,13 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, } else { sgtl5000->micbias_voltage = 0; } + if (of_property_read_bool(np, "sysclk-fixed")) { + sgtl5000->sysclk = clk_get_rate(sgtl5000->mclk); + sgtl5000->sysclk_fixed = 1; + dev_dbg(&client->dev, + "sysclk-fixed: setting sysclk to fixed value %d\n", + sgtl5000->sysclk); + } }
sgtl5000->lrclk_strength = I2S_LRCLK_STRENGTH_LOW; -- 2.11.0
Hi Jakob, On Mon, Oct 16, 2017 at 7:46 AM, Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> wrote:
Hi Fabio, that’s interesting.
Looks like the fsl,imx-audio-sgtl5000 sound card driver that is used on the imx51-babbage does not try to adjust MCLK in response to the playback sample rate.
The simple-audio-card driver does that (it calls snd_soc_dai_set_sysclk whenever playback starts).
Could you please test this with linux-next tree? Looks like that the recent b0a7043d5c2c("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width") would avoid the issue. Regards, Fabio Estevam
On 16. Oct 2017, at 12:51, Fabio Estevam <festevam@gmail.com> wrote:
Hi Jakob,
On Mon, Oct 16, 2017 at 7:46 AM, Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> wrote:
Hi Fabio, that’s interesting.
Looks like the fsl,imx-audio-sgtl5000 sound card driver that is used on the imx51-babbage does not try to adjust MCLK in response to the playback sample rate.
The simple-audio-card driver does that (it calls snd_soc_dai_set_sysclk whenever playback starts).
Could you please test this with linux-next tree?
Looks like that the recent b0a7043d5c2c("ASoC: fsl_ssi: Caculate bit clock rate using slot number and width") would avoid the issue.
Hi Fabio, if this ( https://patchwork.kernel.org/patch/9952385/ ) is the patch, then it seems to be specific to the Freescale I2S controller. We use the SGTL5000 together with a Rockchip CPU. I think I see a way to fix this without adding a DTS property, I’ll send out a patch later this afternoon. Best regards, Jakob
Hi Jakob, On Mon, Oct 16, 2017 at 9:17 AM, Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> wrote:
Hi Fabio,
if this ( https://patchwork.kernel.org/patch/9952385/ ) is the patch, then it seems to be specific to the Freescale I2S controller. We use the SGTL5000 together with a Rockchip CPU.
Yes, it is specific to the SSI interface on i.MX. I thought you were using i.MX, sorry.
I think I see a way to fix this without adding a DTS property, I’ll send out a patch later this afternoon.
Ok, great.
On 16. Oct 2017, at 13:19, Fabio Estevam <festevam@gmail.com> wrote:
I think I see a way to fix this without adding a DTS property, I’ll send out a patch later this afternoon.
Ok, great.
Hi Fabio, I think I found a way to get our use-case working with just DTS configuration for simple-audio-card. When you define the “clocks” property for the "simple-audio-card,cpu” sub-node, set_sysclk is called only once, on startup - like the IMX driver does. We should get away without changes to the codec driver. Thanks & best regards, Jakob
Hi Jakob, On Tue, Oct 17, 2017 at 7:01 AM, Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com> wrote:
Hi Fabio, I think I found a way to get our use-case working with just DTS configuration for simple-audio-card.
When you define the “clocks” property for the "simple-audio-card,cpu” sub-node, set_sysclk is called only once, on startup - like the IMX driver does.
We should get away without changes to the codec driver.
Excellent! This looks better :-)
participants (3)
-
Fabio Estevam -
Fabio Estevam -
Jakob Unterwurzacher