| Message ID | 1418997336-17777-7-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 19, 2014 at 02:55:26PM +0100, Sylwester Nawrocki wrote: > Gating the I2S bus clock in the driver's runtime PM callbacks has > currently really no effect since the clock is being enabled > in the DAI's probe() and thus is permanently turned on. Now we just > move the enable to the platform driver's probe(), which doesn't > change the situation much. It will allow us to register a clock > provider already in samsung_i2s_probe() function. That doesn't sound quite right - the normal idiom for this stuff is to enable on probe, mark the device as active then if runtime PM is enabled it can idle the device and turn off the clock. That way if runtime PM is disabled things continue to run. I've not checked to see if this is actually happening correctly all the way through but that's what's supposed to happen and means that enabling in the probe should still result in working clock management. It will as things currently stand be broken for the dual DAI case so this is a fix for that (the dual DAI case will double enable if both links are in use) but the analysis isn't quite correct.
On 06/01/15 19:06, Mark Brown wrote: > On Fri, Dec 19, 2014 at 02:55:26PM +0100, Sylwester Nawrocki wrote: >> Gating the I2S bus clock in the driver's runtime PM callbacks has >> currently really no effect since the clock is being enabled >> in the DAI's probe() and thus is permanently turned on. Now we just >> move the enable to the platform driver's probe(), which doesn't >> change the situation much. It will allow us to register a clock >> provider already in samsung_i2s_probe() function. > > That doesn't sound quite right - the normal idiom for this stuff is to > enable on probe, mark the device as active then if runtime PM is enabled > it can idle the device and turn off the clock. That way if runtime PM > is disabled things continue to run. I've not checked to see if this is > actually happening correctly all the way through but that's what's > supposed to happen and means that enabling in the probe should still > result in working clock management. > > It will as things currently stand be broken for the dual DAI case so > this is a fix for that (the dual DAI case will double enable if both > links are in use) but the analysis isn't quite correct. I agree that the device should be additionally marked as active, however things will stop working unless we also do other changes to ensure the BUSCLK is enabled when required. AFAIU the clock will not be double enabled when both links are used, since the clk_get/clk_prepare_enable part in DAI probe() is done only for the primary DAI. I actually checked it with single and both links and the clk_enable_count is always 1 after system initialization: # grep . /sys/kernel/debug/clk/*i2s*/clk_enable_count /sys/kernel/debug/clk/dout_i2s/clk_enable_count:0 /sys/kernel/debug/clk/i2s0/clk_enable_count:0 /sys/kernel/debug/clk/i2s1/clk_enable_count:0 /sys/kernel/debug/clk/i2s2/clk_enable_count:0 /sys/kernel/debug/clk/i2s_bus/clk_enable_count:1 /sys/kernel/debug/clk/mout_i2s/clk_enable_count:0 /sys/kernel/debug/clk/sclk_i2s/clk_enable_count:0 /sys/kernel/debug/clk/sclk_i2s1/clk_enable_count:0 /sys/kernel/debug/clk/sclk_i2s2/clk_enable_count:0 I'm going to rephrase the changelog to something like: "The clk_prepare_enable() call on the "iis" clock is moved to happen earlier in the DAI platform device driver's probe() callback, so the I2S registers can be safely accessed through the clk API, after the clk supplier is registered in the platform device probe(). After this patch the "iis" clock is kept enabled since the (primary) I2S platform device probe() and until the platform device driver remove() call. This is similar to gating the clock in the snd_soc_dai probe() and remove() callbacks. Normally, in addition to that we should mark the device as PM runtime active, so if runtime PM is enabled it can idle the device by turning off the clock. Correcting this issue is left for a separate patch series, as we need to ensure the BUSCLK clock is always enabled when required."
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c index 7f967b2..80b5362 100644 --- a/sound/soc/samsung/i2s.c +++ b/sound/soc/samsung/i2s.c @@ -969,7 +969,6 @@ static int samsung_i2s_dai_probe(struct snd_soc_dai *dai) { struct i2s_dai *i2s = to_info(dai); struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai; - int ret; if (is_secondary(i2s)) { /* If this is probe on the secondary DAI */ samsung_asoc_init_dma_data(dai, &other->sec_dai->dma_playback, @@ -977,12 +976,6 @@ static int samsung_i2s_dai_probe(struct snd_soc_dai *dai) goto probe_exit; } - ret = clk_prepare_enable(i2s->clk); - if (ret != 0) { - dev_err(&i2s->pdev->dev, "failed to enable clock: %d\n", ret); - return ret; - } - samsung_asoc_init_dma_data(dai, &i2s->dma_playback, &i2s->dma_capture); if (i2s->quirks & QUIRK_NEED_RSTCLR) @@ -1014,18 +1007,12 @@ probe_exit: static int samsung_i2s_dai_remove(struct snd_soc_dai *dai) { struct i2s_dai *i2s = snd_soc_dai_get_drvdata(dai); - struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai; - - if (!other || !other->clk) { + if (!is_secondary(i2s)) { if (i2s->quirks & QUIRK_NEED_RSTCLR) writel(0, i2s->addr + I2SCON); - - clk_disable_unprepare(i2s->clk); } - i2s->clk = NULL; - return 0; } @@ -1139,6 +1126,7 @@ static int samsung_i2s_probe(struct platform_device *pdev) u32 regs_base, quirks = 0, idma_addr = 0; struct device_node *np = pdev->dev.of_node; const struct samsung_i2s_dai_data *i2s_dai_data; + int ret; /* Call during Seconday interface registration */ i2s_dai_data = samsung_i2s_get_driver_data(pdev); @@ -1216,6 +1204,12 @@ static int samsung_i2s_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Failed to get iis clock\n"); return PTR_ERR(pri_dai->clk); } + + ret = clk_prepare_enable(pri_dai->clk); + if (ret != 0) { + dev_err(&pdev->dev, "failed to enable clock: %d\n", ret); + return ret; + } pri_dai->dma_playback.dma_addr = regs_base + I2STXD; pri_dai->dma_capture.dma_addr = regs_base + I2SRXD; pri_dai->dma_playback.ch_name = "tx"; @@ -1286,6 +1280,9 @@ static int samsung_i2s_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); } + if (!is_secondary(i2s)) + clk_disable_unprepare(i2s->clk); + i2s->pri_dai = NULL; i2s->sec_dai = NULL;
Gating the I2S bus clock in the driver's runtime PM callbacks has currently really no effect since the clock is being enabled in the DAI's probe() and thus is permanently turned on. Now we just move the enable to the platform driver's probe(), which doesn't change the situation much. It will allow us to register a clock provider already in samsung_i2s_probe() function. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- sound/soc/samsung/i2s.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)