SGTL500 and its external MCLK

SGTL500 and its external MCLK

[jonsmirl]

For testing purposes I have a SGTL5000 wired up to I2S on my Allwinner
A20.  SGTL5000 needs an external MCLK. It attaches to it like this:

sgtl5000: sgtl5000@a {
   compatible = "fsl,sgtl5000";
   reg = <0x0a>;
   clocks = <&iis0>;

And in the probe code....
(this should use a name for the clock in the DTS)

sgtl5000->mclk = devm_clk_get(&client->dev, NULL);
if (IS_ERR(sgtl5000->mclk)) {
   ret = PTR_ERR(sgtl5000->mclk);
   dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
   /* Defer the probe to see if the clk will be provided later */
   if (ret == -ENOENT)
      return -EPROBE_DEFER;
   return ret;
}

ret = clk_prepare_enable(sgtl5000->mclk);
if (ret)
   return ret;

So the sgtl5000 driver has a handle to the clock. But then the driver
implements set_dai_sysclk() to get the rate. And a board specific
machine driver like imx-sgtl5000.c is used to set this rate into the
sgtl5000 driver.

/* set codec sysclk */
static int sgtl5000_set_dai_sysclk(struct snd_soc_dai *codec_dai,
  int clk_id, unsigned int freq, int dir)
{
   struct snd_soc_codec *codec = codec_dai->codec;
   struct sgtl5000_priv *sgtl5000 =      snd_soc_codec_get_drvdata(codec);

switch (clk_id) {
   case SGTL5000_SYSCLK:
   sgtl5000->sysclk = freq;
      break;
default:
   return -EINVAL;
  }

  return 0;
}

Since the sgtl5000 driver has the handle to the clock, can't it just
ask the clock for its rate? If it directly asked the clock for its
rate it looks like this codec could be bound with simple-audio-card
and not need a machine driver.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: SGTL500 and its external MCLK

[Nicolin Chen]

On Thu, Aug 07, 2014 at 03:37:47PM -0400, jonsmirl@gmail.com wrote:
> Since the sgtl5000 driver has the handle to the clock, can't it just
> ask the clock for its rate? If it directly asked the clock for its
> rate it looks like this codec could be bound with simple-audio-card
> and not need a machine driver.

I think Simple Card should already have the capability to support
this without changing sgtl5000's code. It has two properties that
can make it call set_sysclk() for you during the init(). They are
'clocks' and 'system-clock-frequency'. Please refer to its binding
doc for details.

And the topic why not let sgtl5000 fully control the clock has been
discussed in this thread:
http://comments.gmane.org/gmane.linux.alsa.devel/109093

So I think the change can be done as well?

Best regards,
Nicolin

Re: SGTL500 and its external MCLK

[jonsmirl]

On Fri, Aug 8, 2014 at 1:29 AM, Nicolin Chen <Guangyu.Chen@freescale.com> wrote:
> On Thu, Aug 07, 2014 at 03:37:47PM -0400, jonsmirl@gmail.com wrote:
>> Since the sgtl5000 driver has the handle to the clock, can't it just
>> ask the clock for its rate? If it directly asked the clock for its
>> rate it looks like this codec could be bound with simple-audio-card
>> and not need a machine driver.
>
> I think Simple Card should already have the capability to support
> this without changing sgtl5000's code. It has two properties that
> can make it call set_sysclk() for you during the init(). They are
> 'clocks' and 'system-clock-frequency'. Please refer to its binding
> doc for details.
>
> And the topic why not let sgtl5000 fully control the clock has been
> discussed in this thread:
> http://comments.gmane.org/gmane.linux.alsa.devel/109093
>
> So I think the change can be done as well?

I tried hacking on it some. One problem is with clocks that aren't
100% accurate. For example my clock is off a little - 22.571428. So
when the code divides that by 44100 it gets 511.8 which truncates to
511 and the test for equal to 512 fails.


>
> Best regards,
> Nicolin



-- 
Jon Smirl
jonsmirl@gmail.com

Re: SGTL500 and its external MCLK

[jonsmirl]

On Fri, Aug 8, 2014 at 1:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Fri, Aug 8, 2014 at 1:29 AM, Nicolin Chen <Guangyu.Chen@freescale.com> wrote:
>> On Thu, Aug 07, 2014 at 03:37:47PM -0400, jonsmirl@gmail.com wrote:
>>> Since the sgtl5000 driver has the handle to the clock, can't it just
>>> ask the clock for its rate? If it directly asked the clock for its
>>> rate it looks like this codec could be bound with simple-audio-card
>>> and not need a machine driver.
>>
>> I think Simple Card should already have the capability to support
>> this without changing sgtl5000's code. It has two properties that
>> can make it call set_sysclk() for you during the init(). They are
>> 'clocks' and 'system-clock-frequency'. Please refer to its binding
>> doc for details.

Simple has a hard coded clock ID of zero. Which just happens to match
codecs/sgtl5000.h:#define SGTL5000_SYSCLK 0x00

Lucky coincidence?

static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
     struct snd_pcm_hw_params *params)
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct snd_soc_dai *codec_dai = rtd->codec_dai;
struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
unsigned int mclk;
int ret = 0;

if (priv->mclk_fs) {
mclk = params_rate(params) * priv->mclk_fs;
ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
    SND_SOC_CLOCK_IN);
}

return ret;
}



>>
>> And the topic why not let sgtl5000 fully control the clock has been
>> discussed in this thread:
>> http://comments.gmane.org/gmane.linux.alsa.devel/109093
>>
>> So I think the change can be done as well?
>
> I tried hacking on it some. One problem is with clocks that aren't
> 100% accurate. For example my clock is off a little - 22.571428. So
> when the code divides that by 44100 it gets 511.8 which truncates to
> 511 and the test for equal to 512 fails.
>
>
>>
>> Best regards,
>> Nicolin
>
>
>
> --
> Jon Smirl
> jonsmirl@gmail.com



-- 
Jon Smirl
jonsmirl@gmail.com

Re: SGTL500 and its external MCLK

[Nicolin Chen]

On Sat, Aug 09, 2014 at 12:42:43PM -0400, jonsmirl@gmail.com wrote:
> On Fri, Aug 8, 2014 at 1:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> > On Fri, Aug 8, 2014 at 1:29 AM, Nicolin Chen <Guangyu.Chen@freescale.com> wrote:
> >> On Thu, Aug 07, 2014 at 03:37:47PM -0400, jonsmirl@gmail.com wrote:
> >>> Since the sgtl5000 driver has the handle to the clock, can't it just
> >>> ask the clock for its rate? If it directly asked the clock for its
> >>> rate it looks like this codec could be bound with simple-audio-card
> >>> and not need a machine driver.
> >>
> >> I think Simple Card should already have the capability to support
> >> this without changing sgtl5000's code. It has two properties that
> >> can make it call set_sysclk() for you during the init(). They are
> >> 'clocks' and 'system-clock-frequency'. Please refer to its binding
> >> doc for details.
> 
> Simple has a hard coded clock ID of zero. Which just happens to match
> codecs/sgtl5000.h:#define SGTL5000_SYSCLK 0x00
> 
> Lucky coincidence?

It's also pretty fair to think that Simple Card only supports CODEC
using the main sysclk -- id is 0x00. And I believe this also works
out for other CODECs whose sysclk configurations aren't complicated
since the name is Simple Card :)

> >> And the topic why not let sgtl5000 fully control the clock has been
> >> discussed in this thread:
> >> http://comments.gmane.org/gmane.linux.alsa.devel/109093
> >>
> >> So I think the change can be done as well?
> >
> > I tried hacking on it some. One problem is with clocks that aren't
> > 100% accurate. For example my clock is off a little - 22.571428. So
> > when the code divides that by 44100 it gets 511.8 which truncates to
> > 511 and the test for equal to 512 fails.

This is a clock dividing accuracy issue. We can try DIV_ROUND_UP()
instead of the truncating division.

Best regards,
Nicolin

Re: SGTL500 and its external MCLK

[jonsmirl]

On Mon, Aug 11, 2014 at 1:50 AM, Nicolin Chen
<Guangyu.Chen@freescale.com> wrote:
> On Sat, Aug 09, 2014 at 12:42:43PM -0400, jonsmirl@gmail.com wrote:
>> On Fri, Aug 8, 2014 at 1:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> > On Fri, Aug 8, 2014 at 1:29 AM, Nicolin Chen <Guangyu.Chen@freescale.com> wrote:
>> >> On Thu, Aug 07, 2014 at 03:37:47PM -0400, jonsmirl@gmail.com wrote:
>> >>> Since the sgtl5000 driver has the handle to the clock, can't it just
>> >>> ask the clock for its rate? If it directly asked the clock for its
>> >>> rate it looks like this codec could be bound with simple-audio-card
>> >>> and not need a machine driver.
>> >>
>> >> I think Simple Card should already have the capability to support
>> >> this without changing sgtl5000's code. It has two properties that
>> >> can make it call set_sysclk() for you during the init(). They are
>> >> 'clocks' and 'system-clock-frequency'. Please refer to its binding
>> >> doc for details.
>>
>> Simple has a hard coded clock ID of zero. Which just happens to match
>> codecs/sgtl5000.h:#define SGTL5000_SYSCLK 0x00
>>
>> Lucky coincidence?
>
> It's also pretty fair to think that Simple Card only supports CODEC
> using the main sysclk -- id is 0x00. And I believe this also works
> out for other CODECs whose sysclk configurations aren't complicated
> since the name is Simple Card :)
>
>> >> And the topic why not let sgtl5000 fully control the clock has been
>> >> discussed in this thread:
>> >> http://comments.gmane.org/gmane.linux.alsa.devel/109093
>> >>
>> >> So I think the change can be done as well?
>> >
>> > I tried hacking on it some. One problem is with clocks that aren't
>> > 100% accurate. For example my clock is off a little - 22.571428. So
>> > when the code divides that by 44100 it gets 511.8 which truncates to
>> > 511 and the test for equal to 512 fails.
>
> This is a clock dividing accuracy issue. We can try DIV_ROUND_UP()
> instead of the truncating division.

Mark - how is clock accuracy supposed to be handled in ALSA? Should my
clocks tell their true rate 22.571428Mhz or lie and say they are
22.5792Mhz?

Are there problems with division truncation from the slightly low
clock rate lurking in the ALSA code that I need to avoid?



>
> Best regards,
> Nicolin



-- 
Jon Smirl
jonsmirl@gmail.com

Re: SGTL500 and its external MCLK

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 604 bytes --]

On Mon, Aug 11, 2014 at 08:29:01AM -0400, jonsmirl@gmail.com wrote:

> Mark - how is clock accuracy supposed to be handled in ALSA? Should my
> clocks tell their true rate 22.571428Mhz or lie and say they are
> 22.5792Mhz?

> Are there problems with division truncation from the slightly low
> clock rate lurking in the ALSA code that I need to avoid?

I don't think ALSA should be having to deal with tiny inaccuracies;
ideally the clock API would have some sort of accuracy management in it
(in much the same way that the regulator API does) but otherwise just
impedence match at the edge of the code.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]