Message-Id: <EA6FEF1C-263E-4529-AC8E-16F50C27030F@theobroma-systems.com>
Date:   Mon, 16 Oct 2017 11:46:44 +0200
From:   Jakob Unterwurzacher <jakob.unterwurzacher@...obroma-systems.com>
To:     Fabio Estevam <fabio.estevam@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Klaus Goger <klaus.goger@...obroma-systems.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Richard Leitner <richard.leitner@...data.com>,
        Bhumika Goyal <bhumirks@...il.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "festevam@...il.com" <festevam@...il.com>
Subject: Re: [PATCH] ASoC: sgtl5000: add support for a fixed sysclk frequency

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@....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@...obroma-systems.com>
> Sent: Friday, October 6, 2017 5:34:44 AM
> To: linux-kernel@...r.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@...a-project.org; devicetree@...r.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@...obroma-systems.com>
> Cc: Klaus Goger <klaus.goger@...obroma-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

Powered by blists - more mailing lists