[alsa-devel] [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
Move the ipg clock enable and disable operation to startup and shutdown, that is only enable ipg clock when ssi is working. we don't need to enable ipg clock in probe. Another register accessing need the ipg clock, so use devm_regmap_init_mmio_clk instead of devm_regmap_init_mmio.
Signed-off-by: Shengjiu Wang shengjiu.wang@freescale.com --- sound/soc/fsl/fsl_ssi.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 2fc3e66..d32d0f5 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -531,6 +531,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai);
+ if (ssi_private->soc->imx) + clk_prepare_enable(ssi_private->clk); + /* When using dual fifo mode, it is safer to ensure an even period * size. If appearing to an odd number while DMA always starts its * task from fifo0, fifo1 would be neglected at the end of each @@ -544,6 +547,22 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, }
/** + * fsl_ssi_shutdown: shutdown the SSI + * + */ +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct fsl_ssi_private *ssi_private = + snd_soc_dai_get_drvdata(rtd->cpu_dai); + + if (ssi_private->soc->imx) + clk_disable_unprepare(ssi_private->clk); + +} + +/** * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock * * Note: This function can be only called when using SSI as DAI master @@ -1043,6 +1062,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { .startup = fsl_ssi_startup, + .shutdown = fsl_ssi_shutdown, .hw_params = fsl_ssi_hw_params, .hw_free = fsl_ssi_hw_free, .set_fmt = fsl_ssi_set_dai_fmt, @@ -1168,16 +1188,10 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, u32 dmas[4]; int ret;
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL); + ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); if (IS_ERR(ssi_private->clk)) { ret = PTR_ERR(ssi_private->clk); - dev_err(&pdev->dev, "could not get clock: %d\n", ret); - return ret; - } - - ret = clk_prepare_enable(ssi_private->clk); - if (ret) { - dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret); + dev_err(&pdev->dev, "could not get ipg clock: %d\n", ret); return ret; }
@@ -1236,7 +1250,6 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, return 0;
error_pcm: - clk_disable_unprepare(ssi_private->clk);
return ret; } @@ -1246,7 +1259,6 @@ static void fsl_ssi_imx_clean(struct platform_device *pdev, { if (!ssi_private->use_dma) imx_pcm_fiq_exit(pdev); - clk_disable_unprepare(ssi_private->clk); }
static int fsl_ssi_probe(struct platform_device *pdev) @@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, + if (ssi_private->soc->imx) + ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev, + "ipg", iomem, &fsl_ssi_regconfig); + else + ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem, &fsl_ssi_regconfig); if (IS_ERR(ssi_private->regs)) { dev_err(&pdev->dev, "Failed to init register map\n");
Hi,
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
Move the ipg clock enable and disable operation to startup and shutdown, that is only enable ipg clock when ssi is working. we don't need to enable ipg clock in probe. Another register accessing need the ipg clock, so use devm_regmap_init_mmio_clk instead of devm_regmap_init_mmio.
Signed-off-by: Shengjiu Wang shengjiu.wang@freescale.com
sound/soc/fsl/fsl_ssi.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 2fc3e66..d32d0f5 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -531,6 +531,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai);
- if (ssi_private->soc->imx)
clk_prepare_enable(ssi_private->clk);- /* When using dual fifo mode, it is safer to ensure an even period
- size. If appearing to an odd number while DMA always starts its
- task from fifo0, fifo1 would be neglected at the end of each
@@ -544,6 +547,22 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, }
/**
- fsl_ssi_shutdown: shutdown the SSI
- */
+static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct fsl_ssi_private *ssi_private =
snd_soc_dai_get_drvdata(rtd->cpu_dai);- if (ssi_private->soc->imx)
clk_disable_unprepare(ssi_private->clk);+}
+/**
- fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
- Note: This function can be only called when using SSI as DAI master
@@ -1043,6 +1062,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
static const struct snd_soc_dai_ops fsl_ssi_dai_ops = { .startup = fsl_ssi_startup,
- .shutdown = fsl_ssi_shutdown, .hw_params = fsl_ssi_hw_params, .hw_free = fsl_ssi_hw_free, .set_fmt = fsl_ssi_set_dai_fmt,
@@ -1168,16 +1188,10 @@ static int fsl_ssi_imx_probe(struct platform_device *pdev, u32 dmas[4]; int ret;
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
- ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
This does not work for most imx SoCs at the moment. imx27, imx35, imx51 etc. do not have clock-names defined in the devicetree.
Best regards,
Markus
Hi,
Subject: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
Move the ipg clock enable and disable operation to startup and shutdown, that is only enable ipg clock when ssi is working. we don't need to enable ipg clock in probe. Another register accessing need the ipg clock, so use devm_regmap_init_mmio_clk instead of devm_regmap_init_mmio.
You should split this into two separate patches, which will be more Easy to be reviewed.
Thanks,
BRs Xiubo
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
- if (ssi_private->soc->imx)
clk_prepare_enable(ssi_private->clk);
We're ignoring the error code here.
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
- ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); if (IS_ERR(ssi_private->clk)) { ret = PTR_ERR(ssi_private->clk);
dev_err(&pdev->dev, "could not get clock: %d\n", ret);return ret;
Why is this change being made? It wasn't mentioned in the commit log and doesn't seem relevant to moving where the enable and disable are done which is what the patch is supposed to be doing...
Please don't make different changes in the same patch.
On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
- ssi_private->clk = devm_clk_get(&pdev->dev, "ipg"); if (IS_ERR(ssi_private->clk)) { ret = PTR_ERR(ssi_private->clk);
dev_err(&pdev->dev, "could not get clock: %d\n", ret);return ret;Why is this change being made? It wasn't mentioned in the commit log and doesn't seem relevant to moving where the enable and disable are done which is what the patch is supposed to be doing...
I think Shengjiu is trying to keep the clock disabled while SSI's idle. The current driver enables ipg clock anyway even if there's no stream running.
Apparently, these should be put into the comment log.
Thank you Nicolin
On Tue, Sep 09, 2014 at 11:03:10AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
- ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
Why is this change being made? It wasn't mentioned in the commit log and doesn't seem relevant to moving where the enable and disable are done which is what the patch is supposed to be doing...
I think Shengjiu is trying to keep the clock disabled while SSI's idle. The current driver enables ipg clock anyway even if there's no stream running.
Apparently, these should be put into the comment log.
I got that bit. However as well as changing where the enable and disable take place this is also changing from requesting a clock with a NULL to requesting one called "ipg".
On Tue, Sep 09, 2014 at 07:15:16PM +0100, Mark Brown wrote:
On Tue, Sep 09, 2014 at 11:03:10AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
- ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
- ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");
Why is this change being made? It wasn't mentioned in the commit log and doesn't seem relevant to moving where the enable and disable are done which is what the patch is supposed to be doing...
I think Shengjiu is trying to keep the clock disabled while SSI's idle. The current driver enables ipg clock anyway even if there's no stream running.
Apparently, these should be put into the comment log.
I got that bit. However as well as changing where the enable and disable take place this is also changing from requesting a clock with a NULL to requesting one called "ipg".
Understood. Making one patch do one single change is the rule we should always follow.
On Tue, Sep 09, 2014 at 08:17:51AM -0500, Timur Tabi wrote:
Shengjiu Wang wrote:
- if (ssi_private->soc->imx)
clk_prepare_enable(ssi_private->clk);
How about this instead?
if (ssi_private->clk) clk_prepare_enable(ssi_private->clk);
Should be a !IS_ERR() - NULL is a valid clock.
On 09/09/2014 10:21 AM, Mark Brown wrote:
if (ssi_private->clk)
clk_prepare_enable(ssi_private->clk);
Should be a !IS_ERR() - NULL is a valid clock.
In that case, ssi_private->clk needs to be initialized to -EINVAL or something, so that the check works on systems that don't have any clocks.
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() { + clk_prepare_enable(); .... }
close() { .... + clk_disable_unprepare() }
probe() { clk_get(); clk_prepare_enable(); .... if (xxx) - goto err_xx; + return ret; .... + clk_disable_unprepare(); return 0; -err_xx: - clk_disable_unprepare() }
remove() { .... - clk_disable_unprepare() }
As long as you make the subject clear as 'Don't enable core/ipg clock when SSI's idle', I'm sure you can make them within a single patch.
And an alternative way for open() and close() is to put those code into pm_runtime_resume/suspend() instead (since we might have some internal code need to be added by using pm_runtime as well), which would make the further code neater IMO.
Thank you Nicolin
On 09/09/2014 01:38 PM, Nicolin Chen wrote:
make sure to have the call for imx only because it seems that the other platforms do not depend on the clock.
Although I doubt anyone will every add support for clocks to PowerPC "side" of this driver, I would prefer to avoid IMX-specific changes. Instead, the code should check if a clock is available. That's why I suggested this change:
- if (ssi_private->soc->imx) + if (!IS_ERR(ssi_private->clk))
On Tue, Sep 09, 2014 at 02:37:42PM -0500, Timur Tabi wrote:
On 09/09/2014 01:38 PM, Nicolin Chen wrote:
make sure to have the call for imx only because it seems that the other platforms do not depend on the clock.
Although I doubt anyone will every add support for clocks to PowerPC "side" of this driver, I would prefer to avoid IMX-specific changes. Instead, the code should check if a clock is available. That's why I suggested this change:
- if (ssi_private->soc->imx)
- if (!IS_ERR(ssi_private->clk))
Hmm.... I think the following change may be better?
probe() { .... + /* + * Initially mark the clock to NULL for all platforms so that later + * clk_prepare_enable() will ignore and return 0 for non-clock cases. + */ + ssi_private->clk = NULL; ..... fsl_ssi_imx_probe(); }
In this way, all platforms, not confined to imx any more, will be able to call clk_prepare_enable(). Then we don't need an extra platform check before calling it.
On 09/09/2014 02:59 PM, Nicolin Chen wrote:
- /*
* Initially mark the clock to NULL for all platforms so that later* clk_prepare_enable() will ignore and return 0 for non-clock cases.*/- ssi_private->clk = NULL;
According to Mark, NULL is a valid clock, so this should be instead:
ssi_private->clk = PTR_ERR(-EINVAL);
although that doesn't sit well with me.
On Tue, Sep 09, 2014 at 03:03:53PM -0500, Timur Tabi wrote:
On 09/09/2014 02:59 PM, Nicolin Chen wrote:
- /*
* Initially mark the clock to NULL for all platforms so that later* clk_prepare_enable() will ignore and return 0 for non-clock cases.*/- ssi_private->clk = NULL;
According to Mark, NULL is a valid clock, so this should be instead:
ssi_private->clk = PTR_ERR(-EINVAL);
although that doesn't sit well with me.
I guess Mark's comment is merely against the check for clk validation because if talking about clk validation, we should check IS_ERR(clk) rather than check !=NULL directly.
However, my approach doesn't need any check. The open() or pm_resume() can just call clk_prepare_enable() directly. The __clk_enable() will then handle the 'clk == NULL' case:
static int __clk_enable(struct clk *clk) { int ret = 0;
if (!clk) return 0;
On 09/09/2014 03:27 PM, Nicolin Chen wrote:
I guess Mark's comment is merely against the check for clk validation because if talking about clk validation, we should check IS_ERR(clk) rather than check !=NULL directly.
Ah, that makes sense now.
However, my approach doesn't need any check. The open() or pm_resume() can just call clk_prepare_enable() directly. The __clk_enable() will then handle the 'clk == NULL' case:
Yes, I was thinking the same thing.
On Tue, Sep 09, 2014 at 03:37:26PM -0500, Timur Tabi wrote:
However, my approach doesn't need any check. The open() or pm_resume() can just call clk_prepare_enable() directly. The __clk_enable() will then handle the 'clk == NULL' case:
Yes, I was thinking the same thing.
Because that's following your suggestion :)
@Shengjiu Another thing I forgot to mention is we still need a return check for clk_prepare_enable() which isn't in the current version. And I said "doesn't need any check" is indicating the pre-check of the call.
Thank you Nicolin
On Tue, Sep 09, 2014 at 12:59:29PM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 02:37:42PM -0500, Timur Tabi wrote:
On 09/09/2014 01:38 PM, Nicolin Chen wrote:
make sure to have the call for imx only because it seems that the other platforms do not depend on the clock.
Although I doubt anyone will every add support for clocks to PowerPC "side" of this driver, I would prefer to avoid IMX-specific changes. Instead, the code should check if a clock is available. That's why I suggested this change:
- if (ssi_private->soc->imx)
- if (!IS_ERR(ssi_private->clk))
Hmm.... I think the following change may be better?
probe() { ....
- /*
* Initially mark the clock to NULL for all platforms so that later* clk_prepare_enable() will ignore and return 0 for non-clock cases.*/- ssi_private->clk = NULL; ..... fsl_ssi_imx_probe();
}
ssi_private is initialized to zero in beginning of probe. I think no need to add this change here.
wang shengjiu
In this way, all platforms, not confined to imx any more, will be able to call clk_prepare_enable(). Then we don't need an extra platform check before calling it.
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
remove() { ....
- clk_disable_unprepare()
}
If I remember correctly, there may be AC97 communication with the codec before any substream is created. That's why we enable the SSI unit right at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to check for AC97 before disabling clocks.
Best regards,
Markus
On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
remove() { ....
- clk_disable_unprepare()
}
If I remember correctly, there may be AC97 communication with the codec before any substream is created. That's why we enable the SSI unit right at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to check for AC97 before disabling clocks.
Thank you for the input. That's the exact part I couldn't be sure. And I agree that adding a check for AC97 will be safer while this may keeps itself removable if being unnecessary.
Thanks Nicolin
On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
remove() { ....
- clk_disable_unprepare()
}
If I remember correctly, there may be AC97 communication with the codec before any substream is created. That's why we enable the SSI unit right at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to check for AC97 before disabling clocks.
Best regards,
Markus
hi Markus
I think if clk_prepare_enable() in startup(), and clk_disable_unprepare() in shutdown can meet this requirement, right?
done: if (ssi_private->dai_fmt) _fsl_ssi_set_dai_fmt(ssi_private, ssi_private->dai_fmt);
I find that in end of probe, there is setting of dai_fmt. Can we remove this? because this setting need to enable ipg clock, and if ac97, ipg clock can't be disabled.
wang shengjiu
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi,
On Wed, Sep 10, 2014 at 06:30:06PM +0800, Shengjiu Wang wrote:
On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
remove() { ....
- clk_disable_unprepare()
}
If I remember correctly, there may be AC97 communication with the codec before any substream is created. That's why we enable the SSI unit right at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to check for AC97 before disabling clocks.
Best regards,
Markus
hi Markus
I think if clk_prepare_enable() in startup(), and clk_disable_unprepare() in shutdown can meet this requirement, right?
Yes that could work.
done: if (ssi_private->dai_fmt) _fsl_ssi_set_dai_fmt(ssi_private, ssi_private->dai_fmt);
I find that in end of probe, there is setting of dai_fmt. Can we remove this? because this setting need to enable ipg clock, and if ac97, ipg clock can't be disabled.
No you can't remove it. It is necessary for the DT property "fsl,mode". Most dts do not have this property anymore because the sound cards are setting the dai-fmt. But there are still some powerpc dts files that contain that property.
Best regards,
Markus
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device *pdev) return -ENOMEM; }
- ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,
- if (ssi_private->soc->imx)
ssi_private->regs = devm_regmap_init_mmio_clk(&pdev->dev,"ipg", iomem, &fsl_ssi_regconfig);- else
ssi_private->regs = devm_regmap_init_mmio(&pdev->dev, iomem,As Markus mentioned, the key point here is to be compatible with those non-clock-name platforms.
I think it would be safer to keep the current code while adding an extra clk_disable_unprepare() at the end of probe() as a common routine. And meantime, make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. //a bit guessing here :)
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
what is the open() and close()? do you mean the fsl_ssi_startup() and fsl_ssi_shutdown()?
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
If this probe() is fsl_ssi_imx_probe(), I think no need to add clk_prepare_enable() or clk_disable_unprepare(), seems there is no registers accessing in this probe.
remove() { ....
- clk_disable_unprepare()
}
As long as you make the subject clear as 'Don't enable core/ipg clock when SSI's idle', I'm sure you can make them within a single patch.
And an alternative way for open() and close() is to put those code into pm_runtime_resume/suspend() instead (since we might have some internal code need to be added by using pm_runtime as well), which would make the further code neater IMO.
Thank you Nicolin
On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
what is the open() and close()? do you mean the fsl_ssi_startup() and fsl_ssi_shutdown()?
Yea.
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
If this probe() is fsl_ssi_imx_probe(), I think no need to add clk_prepare_enable() or clk_disable_unprepare(), seems there is no registers accessing in this probe.
This is trying to be safe, especially for such a driver being used by multiple platforms. You can omit this as long as the patch can pass the test on old imx, PowerPC, and AC97 platforms.
And another risk just came to my mind is that there would be a possibility that a machine driver would call set_dai_fmt() early, after SSI's probe() and before SSI's startup(), if the machine driver contains dai_fmt assignment in its probe(). Then, without regmap_mmio_clk(), it'll be tough for us over here because we may also need to add clock enable/disable for set_dai_fmt/set_sysclk(), even if there might be still tiny risk that we missed something.
Then there could be a selfish approach to circumvent it is to use regmap_mmio_clk() with "ipg" at the beginning and call regmap_mmio() without "ipg" if getting a failed return value from regmap_mmio_clk, and meanwhile to keep the clock always enabled for the regmap_mmio() case just like what the current driver is doing. This may result those non-ipg-clk platforms can't benefit from this refinement unless they update their DT bindings -- use "ipg" for core clock This might be the safest and simplest way for us, I'm not sure everyone would be comfortable with this idea though.
Best regards, Nicolin
On Wed, Sep 10, 2014 at 10:42:04AM -0700, Nicolin Chen wrote:
On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
what is the open() and close()? do you mean the fsl_ssi_startup() and fsl_ssi_shutdown()?
Yea.
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
If this probe() is fsl_ssi_imx_probe(), I think no need to add clk_prepare_enable() or clk_disable_unprepare(), seems there is no registers accessing in this probe.
This is trying to be safe, especially for such a driver being used by multiple platforms. You can omit this as long as the patch can pass the test on old imx, PowerPC, and AC97 platforms.
And another risk just came to my mind is that there would be a possibility that a machine driver would call set_dai_fmt() early, after SSI's probe() and before SSI's startup(), if the machine driver contains dai_fmt assignment in its probe(). Then, without regmap_mmio_clk(), it'll be tough for us over here because we may also need to add clock enable/disable for set_dai_fmt/set_sysclk(), even if there might be still tiny risk that we missed something.
Thanks, didn't thought about that. As there are no restrictions on when these functions may be called, it has to be handled.
Then there could be a selfish approach to circumvent it is to use regmap_mmio_clk() with "ipg" at the beginning and call regmap_mmio() without "ipg" if getting a failed return value from regmap_mmio_clk, and meanwhile to keep the clock always enabled for the regmap_mmio() case just like what the current driver is doing. This may result those non-ipg-clk platforms can't benefit from this refinement unless they update their DT bindings -- use "ipg" for core clock This might be the safest and simplest way for us, I'm not sure everyone would be comfortable with this idea though.
I like the "selfish" approach. It would save a lot of clock enabling/disabling and error handling and at the same time it doesn't break the DT compatibility. The platforms with an old DT would have the old behaviour, but that could be changed by updating the devicetrees which should be easy to do for all the imx SoCs.
Best regards,
Markus
On Thu, Sep 11, 2014 at 08:36:51AM +0200, Markus Pargmann wrote:
On Wed, Sep 10, 2014 at 10:42:04AM -0700, Nicolin Chen wrote:
On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:
Then we can get a patch like: open() {
- clk_prepare_enable(); ....
}
close() { ....
- clk_disable_unprepare()
}
what is the open() and close()? do you mean the fsl_ssi_startup() and fsl_ssi_shutdown()?
Yea.
probe() { clk_get(); clk_prepare_enable(); .... if (xxx)
goto err_xx;
return ret;- clk_disable_unprepare(); return 0;
-err_xx:
- clk_disable_unprepare()
}
If this probe() is fsl_ssi_imx_probe(), I think no need to add clk_prepare_enable() or clk_disable_unprepare(), seems there is no registers accessing in this probe.
This is trying to be safe, especially for such a driver being used by multiple platforms. You can omit this as long as the patch can pass the test on old imx, PowerPC, and AC97 platforms.
And another risk just came to my mind is that there would be a possibility that a machine driver would call set_dai_fmt() early, after SSI's probe() and before SSI's startup(), if the machine driver contains dai_fmt assignment in its probe(). Then, without regmap_mmio_clk(), it'll be tough for us over here because we may also need to add clock enable/disable for set_dai_fmt/set_sysclk(), even if there might be still tiny risk that we missed something.
Thanks, didn't thought about that. As there are no restrictions on when these functions may be called, it has to be handled.
Then there could be a selfish approach to circumvent it is to use regmap_mmio_clk() with "ipg" at the beginning and call regmap_mmio() without "ipg" if getting a failed return value from regmap_mmio_clk, and meanwhile to keep the clock always enabled for the regmap_mmio() case just like what the current driver is doing. This may result those non-ipg-clk platforms can't benefit from this refinement unless they update their DT bindings -- use "ipg" for core clock This might be the safest and simplest way for us, I'm not sure everyone would be comfortable with this idea though.
I like the "selfish" approach. It would save a lot of clock enabling/disabling and error handling and at the same time it doesn't break the DT compatibility. The platforms with an old DT would have the old behaviour, but that could be changed by updating the devicetrees which should be easy to do for all the imx SoCs.
Best regards,
Markus
Thanks Markus and Nicolin
I have sent version 2 for this patch. Please review it.
best regards wang shengjiu
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
participants (6)
-
Li.Xiubo＠freescale.com -
Mark Brown -
Markus Pargmann -
Nicolin Chen -
Shengjiu Wang -
Timur Tabi