Re: [PATCH] sound/soc/adi/axi-spdif.c: Support programmable master clock

From: Lars-Peter Clausen
Date: Thu Dec 04 2014 - 11:54:52 EST

On 12/04/2014 03:18 PM, Mike Looijmans wrote:
On 12/04/2014 01:45 PM, Lars-Peter Clausen wrote:
On 12/04/2014 07:52 AM, Mike Looijmans wrote:
If the master clock supports programmable rates, program it to generate
the desired frequency. Only apply constraints when the clock is fixed.
This allows proper clock generation for both 44100 and 48000 Hz based
sampling rates if the platform supports it.

The clock frequency must be set before enabling it. Enabling the clock
was done in "startup", but that occurs before "hw_params" where the rate
is known. Move the clock start to the hw_params routine, and keep track
of whether the clock has been started, because shutdown may be called
without having called hw_params first.

Usually that shouldn't be a problem. If your clock chip requires it to be
disabled in order to be reprogrammed than the CLK_SET_RATE_GATE flag
should be
set. This will tell the core to disable the clock before changing it.

The issue here is not the clock's capabilities, but the order in which
things are being done. If the driver just enables the clock without ever
having set a rate, the clock will run at whatever happens to be the default.
That default may have unpredictable results or even be harmful to the
system. So the driver should first set a valid clock rate before enabling
the clock. Suggestions on rewording my comments to better reflect that are

static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
@@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device
spdif->dma_data.addr_width = 4;
spdif->dma_data.maxburst = 1;

- spdif->ratnum.num = clk_get_rate(spdif->clk_ref) / 128;
- spdif->ratnum.den_step = 1;
- spdif->ratnum.den_min = 1;
- spdif->ratnum.den_max = 64;
- spdif->rate_constraints.rats = &spdif->ratnum;
- spdif->rate_constraints.nrats = 1;
+ /* Determine if the clock rate is fixed. If it cannot change frequency,
+ * it returns an error here. */
+ if (clk_round_rate(spdif->clk_ref, 128 * 44100) < 0) {

I don't think this works. For a fixed clock clk_round_rate() will return the
fixed rate rather than an error. I tried the patch and even though I have a
fixed clock the constraints are no longer set.

There is unfortunately no good way to enumerate which frequencies are
supported by a clock other than just calling round_rate for all possible

I think the best way to implement this for now is to try e.g. 32000 * 128,
44100 * 128, 48000 * 128 and then check if clk_round_rate returns the
rate and if it does set up a rate constraint for that rate.

For what I could see, ALSA never reported or limited the sample rate
correctly even before this patch, so maybe the even simpler approach is
better: Just remove the constraint. I wonder how you concluded that the
constraint didn't get added?

Newer versions of aplay e.g. support printing the initial constraints:;a=blob;f=aplay/aplay.c;h=e58e1bcbdd7e5c784b85df90f15ef41326d3f6dd;hb=HEAD#l240

And on older versions you can use -v to print the final constraints. E.g. without your patch I see "Actual rate: 96000/2 (48000)" and with your patch just "Actual rate: 48000".

The application you are using might just simply choose to ignore the constraints. But there are definitely applications which honer them which will break if the constraints are removed.

Actually, the SPDIF logic wants a clock that is an "integer multiple of
128*samplerate in the range of 1..64". Other than just looping through them
all, there's also no way to request the clock framework for such a requirement.

Your suggestion is quite robust though, but the fixed clock may be set to
any integer multiple of the desired frequency, so the algorithm would not
work if the fixed clock is running at 48000*256 (12.8MHz like in the
reference design). So I guess the best I could do here is just check that:

clk_round_rate(spdif->clk_ref, 128 * 44100) !=
clk_round_rate(spdif->clk_ref, 128 * 48000)

Sounds like a OK compromise.

- Lars

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at