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

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

[Mike Looijmans]

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.

Starting the clock and enabling the SPDIF output AFTER programming the
dividers is a more logical order anyway.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 sound/soc/adi/axi-spdif.c |   56 ++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/sound/soc/adi/axi-spdif.c b/sound/soc/adi/axi-spdif.c
index 198e3a4..d67e010 100644
--- a/sound/soc/adi/axi-spdif.c
+++ b/sound/soc/adi/axi-spdif.c
@@ -4,7 +4,6 @@
  *
  * Licensed under the GPL-2.
  */
-
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -44,6 +43,8 @@ struct axi_spdif {
 
 	struct snd_ratnum ratnum;
 	struct snd_pcm_hw_constraint_ratnums rate_constraints;
+
+	bool clk_ref_running;
 };
 
 static int axi_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
@@ -79,6 +80,7 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	unsigned int rate = params_rate(params);
 	unsigned int clkdiv, stat;
+	int ret;
 
 	switch (params_rate(params)) {
 	case 32000:
@@ -95,6 +97,9 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
+	/* Try to set the master clock */
+	clk_set_rate(spdif->clk_ref, rate * 128);
+
 	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
 			rate * 64 * 2) - 1;
 	clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
@@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
 
+	ret = clk_prepare_enable(spdif->clk_ref);
+	if (ret)
+		return ret;
+	spdif->clk_ref_running = true;
+
+	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
+		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+
 	return 0;
 }
 
@@ -121,18 +134,13 @@ static int axi_spdif_startup(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	int ret;
 
-	ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
-			   SNDRV_PCM_HW_PARAM_RATE,
-			   &spdif->rate_constraints);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(spdif->clk_ref);
-	if (ret)
-		return ret;
-
-	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
-		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+	if (spdif->rate_constraints.nrats) {
+		ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
+				SNDRV_PCM_HW_PARAM_RATE,
+				&spdif->rate_constraints);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -145,7 +153,10 @@ static void axi_spdif_shutdown(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_TXEN, 0);
 
-	clk_disable_unprepare(spdif->clk_ref);
+	if (spdif->clk_ref_running) {
+		clk_disable_unprepare(spdif->clk_ref);
+		spdif->clk_ref_running = false;
+	}
 }
 
 static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
@@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device *pdev)
 	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) {
+		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;
+	}
 	ret = devm_snd_soc_register_component(&pdev->dev, &axi_spdif_component,
 					 &axi_spdif_dai, 1);
 	if (ret)
-- 
1.7.9.5

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

[Lars-Peter Clausen]

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.

[...]
>   static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
> @@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device *pdev)
>   	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 rates.

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 
expected rate and if it does set up a rate constraint for that rate.

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

[Mike Looijmans]

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 welcome.

>
> [...]
>>   static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
>> @@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device *pdev)
>>       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 rates.
>
> 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 expected
> 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?

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)

Only a configurable clock that is compatible with the patch would do that.




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

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

[Lars-Peter Clausen]

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
> welcome.
>
>>
>> [...]
>>>   static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
>>> @@ -216,14 +227,17 @@ static int axi_spdif_probe(struct platform_device
>>> *pdev)
>>>       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
>> rates.
>>
>> 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
>> expected
>> 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:
http://git.alsa-project.org/?p=alsa-utils.git;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

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

[Mike Looijmans]

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. Enabling a programmable clock without first setting a valid
frequency may harm the system. 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.
Starting the clock and enabling the SPDIF output AFTER programming the
dividers is a more logical order anyway.

To detect if the source clock is fixed, the driver calls clk_round_rate
for two frequencies. If the results are equal, or if the call returns
an error, the driver assumes the clock is fixed.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

v2: Fix fixed clock detection as discussed.

 sound/soc/adi/axi-spdif.c |   60 ++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/sound/soc/adi/axi-spdif.c b/sound/soc/adi/axi-spdif.c
index 198e3a4..8fd43a7 100644
--- a/sound/soc/adi/axi-spdif.c
+++ b/sound/soc/adi/axi-spdif.c
@@ -4,7 +4,6 @@
  *
  * Licensed under the GPL-2.
  */
-
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -44,6 +43,8 @@ struct axi_spdif {
 
 	struct snd_ratnum ratnum;
 	struct snd_pcm_hw_constraint_ratnums rate_constraints;
+
+	bool clk_ref_running;
 };
 
 static int axi_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
@@ -79,6 +80,7 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	unsigned int rate = params_rate(params);
 	unsigned int clkdiv, stat;
+	int ret;
 
 	switch (params_rate(params)) {
 	case 32000:
@@ -95,6 +97,9 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
+	/* Try to set the master clock */
+	clk_set_rate(spdif->clk_ref, rate * 128);
+
 	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
 			rate * 64 * 2) - 1;
 	clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
@@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
 
+	ret = clk_prepare_enable(spdif->clk_ref);
+	if (ret)
+		return ret;
+	spdif->clk_ref_running = true;
+
+	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
+		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+
 	return 0;
 }
 
@@ -121,18 +134,13 @@ static int axi_spdif_startup(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	int ret;
 
-	ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
-			   SNDRV_PCM_HW_PARAM_RATE,
-			   &spdif->rate_constraints);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(spdif->clk_ref);
-	if (ret)
-		return ret;
-
-	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
-		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+	if (spdif->rate_constraints.nrats) {
+		ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
+				SNDRV_PCM_HW_PARAM_RATE,
+				&spdif->rate_constraints);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -145,7 +153,10 @@ static void axi_spdif_shutdown(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_TXEN, 0);
 
-	clk_disable_unprepare(spdif->clk_ref);
+	if (spdif->clk_ref_running) {
+		clk_disable_unprepare(spdif->clk_ref);
+		spdif->clk_ref_running = false;
+	}
 }
 
 static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
@@ -183,6 +194,7 @@ static int axi_spdif_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *base;
 	int ret;
+	long rate;
 
 	spdif = devm_kzalloc(&pdev->dev, sizeof(*spdif), GFP_KERNEL);
 	if (!spdif)
@@ -216,14 +228,18 @@ static int axi_spdif_probe(struct platform_device *pdev)
 	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 or it will simply return its fixed value. */
+	rate = clk_round_rate(spdif->clk_ref, 128 * 44100);
+	if (rate < 0 || rate != clk_round_rate(spdif->clk_ref, 128 * 48000)) {
+		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;
+	}
 	ret = devm_snd_soc_register_component(&pdev->dev, &axi_spdif_component,
 					 &axi_spdif_dai, 1);
 	if (ret)
-- 
1.7.9.5

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

[Lars-Peter Clausen]

On 12/05/2014 01:37 PM, 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. Enabling a programmable clock without first setting a valid
> frequency may harm the system. 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.
> Starting the clock and enabling the SPDIF output AFTER programming the
> dividers is a more logical order anyway.
>
> To detect if the source clock is fixed, the driver calls clk_round_rate
> for two frequencies. If the results are equal, or if the call returns
> an error, the driver assumes the clock is fixed.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Hi,

Sorry for the delay.

[...]
>
> +	/* Try to set the master clock */
> +	clk_set_rate(spdif->clk_ref, rate * 128);
> +
>   	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
>   			rate * 64 * 2) - 1;
>   	clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
> @@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
>   	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
>   		AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
>
> +	ret = clk_prepare_enable(spdif->clk_ref);

I'm still not convinced this is the right place. I do see your point. But it 
just feels wrong to enable the clock in hw_params. It's a bit of a dilemma. 
the startup callback is to early, hw_params is the wrong place and we can't 
put it in the trigger callback as the trigger callback can not sleep.

But in any way hwparmas can be called multiple times, so you need to handle 
the case where the clock is already enabled.

> +	if (ret)
> +		return ret;
> +	spdif->clk_ref_running = true;
> +
> +	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
> +		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);

This should probably be moved to the trigger callback though.

> +
>   	return 0;
>   }

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

[Mike Looijmans]

On 12/10/2014 10:34 AM, Lars-Peter Clausen wrote:
> On 12/05/2014 01:37 PM, 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. Enabling a programmable clock without first setting a valid
>> frequency may harm the system. 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.
>> Starting the clock and enabling the SPDIF output AFTER programming the
>> dividers is a more logical order anyway.
>>
>> To detect if the source clock is fixed, the driver calls clk_round_rate
>> for two frequencies. If the results are equal, or if the call returns
>> an error, the driver assumes the clock is fixed.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>
> Hi,
>
> Sorry for the delay.
>
> [...]
>>
>> +    /* Try to set the master clock */
>> +    clk_set_rate(spdif->clk_ref, rate * 128);
>> +
>>       clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
>>               rate * 64 * 2) - 1;
>>       clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
>> @@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream
>> *substream,
>>       regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
>>           AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
>>
>> +    ret = clk_prepare_enable(spdif->clk_ref);
>
> I'm still not convinced this is the right place. I do see your point. But it
> just feels wrong to enable the clock in hw_params. It's a bit of a dilemma.
> the startup callback is to early, hw_params is the wrong place and we can't
> put it in the trigger callback as the trigger callback can not sleep.

The clock interface suggests that we should do the clk_prepare in hw_params 
and the clk_enable in trigger then, since clk_prepare may sleep but clk_enable 
cannot. Which would complicate the clock state housekeeping because I'd have 
to keep track of prepare and enable states separately.

(Imagine the housekeeping on a board that could have two codecs running on one 
DAI using the clock generated by a third codec... I never even bothered to 
submit that...)

> But in any way hwparmas can be called multiple times, so you need to handle
> the case where the clock is already enabled.

A simple "if (spdif->clk_ref_running)" check would fix that. I wasn't aware of 
that though, I thought there'd be a shutdown before another hw_params.

>> +    if (ret)
>> +        return ret;
>> +    spdif->clk_ref_running = true;
>> +
>> +    regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
>> +        AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
>
> This should probably be moved to the trigger callback though.


I'll create a v3.




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

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

[Mike Looijmans]

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. Enabling a programmable clock without first setting a valid
frequency may harm the system. 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, and
hw_params may be called multiple times.
Starting the clock and enabling the SPDIF output AFTER programming the
dividers is a more logical order anyway.

To detect if the source clock is fixed, the driver calls clk_round_rate
for two frequencies. If the results are equal, or if the call returns
an error, the driver assumes the clock is fixed.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v3: Only enable clock once in hw_params which may be called multiple times.

 sound/soc/adi/axi-spdif.c |   62 +++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/sound/soc/adi/axi-spdif.c b/sound/soc/adi/axi-spdif.c
index 198e3a4..d8a98a9 100644
--- a/sound/soc/adi/axi-spdif.c
+++ b/sound/soc/adi/axi-spdif.c
@@ -4,7 +4,6 @@
  *
  * Licensed under the GPL-2.
  */
-
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -44,6 +43,8 @@ struct axi_spdif {
 
 	struct snd_ratnum ratnum;
 	struct snd_pcm_hw_constraint_ratnums rate_constraints;
+
+	bool clk_ref_running;
 };
 
 static int axi_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
@@ -79,6 +80,7 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	unsigned int rate = params_rate(params);
 	unsigned int clkdiv, stat;
+	int ret;
 
 	switch (params_rate(params)) {
 	case 32000:
@@ -95,6 +97,9 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
+	/* Try to set the master clock */
+	clk_set_rate(spdif->clk_ref, rate * 128);
+
 	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
 			rate * 64 * 2) - 1;
 	clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
@@ -103,6 +108,16 @@ static int axi_spdif_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
 
+	if (!spdif->clk_ref_running) {
+		ret = clk_prepare_enable(spdif->clk_ref);
+		if (ret)
+			return ret;
+		spdif->clk_ref_running = true;
+	}
+
+	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
+		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+
 	return 0;
 }
 
@@ -121,18 +136,13 @@ static int axi_spdif_startup(struct snd_pcm_substream *substream,
 	struct axi_spdif *spdif = snd_soc_dai_get_drvdata(dai);
 	int ret;
 
-	ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
-			   SNDRV_PCM_HW_PARAM_RATE,
-			   &spdif->rate_constraints);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(spdif->clk_ref);
-	if (ret)
-		return ret;
-
-	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
-		AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
+	if (spdif->rate_constraints.nrats) {
+		ret = snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
+				SNDRV_PCM_HW_PARAM_RATE,
+				&spdif->rate_constraints);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -145,7 +155,10 @@ static void axi_spdif_shutdown(struct snd_pcm_substream *substream,
 	regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
 		AXI_SPDIF_CTRL_TXEN, 0);
 
-	clk_disable_unprepare(spdif->clk_ref);
+	if (spdif->clk_ref_running) {
+		clk_disable_unprepare(spdif->clk_ref);
+		spdif->clk_ref_running = false;
+	}
 }
 
 static const struct snd_soc_dai_ops axi_spdif_dai_ops = {
@@ -183,6 +196,7 @@ static int axi_spdif_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *base;
 	int ret;
+	long rate;
 
 	spdif = devm_kzalloc(&pdev->dev, sizeof(*spdif), GFP_KERNEL);
 	if (!spdif)
@@ -216,14 +230,18 @@ static int axi_spdif_probe(struct platform_device *pdev)
 	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 or it will simply return its fixed value. */
+	rate = clk_round_rate(spdif->clk_ref, 128 * 44100);
+	if (rate < 0 || rate != clk_round_rate(spdif->clk_ref, 128 * 48000)) {
+		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;
+	}
 	ret = devm_snd_soc_register_component(&pdev->dev, &axi_spdif_component,
 					 &axi_spdif_dai, 1);
 	if (ret)
-- 
1.7.9.5

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

[Lars-Peter Clausen]

On 12/11/2014 08:44 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. Enabling a programmable clock without first setting a valid
> frequency may harm the system. 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, and
> hw_params may be called multiple times.
> Starting the clock and enabling the SPDIF output AFTER programming the
> dividers is a more logical order anyway.
>
> To detect if the source clock is fixed, the driver calls clk_round_rate
> for two frequencies. If the results are equal, or if the call returns
> an error, the driver assumes the clock is fixed.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Thanks.