| Message ID | 1417783029-22492-1-git-send-email-mike.looijmans@topic.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
?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/
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)
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(-)