[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 <redacted>
---
 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");

-- 
1.7.9.5

Hi,

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

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

-- 
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,

Subject: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
=20
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 enabl=

e

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.
=20
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.

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);

On Tue, Sep 09, 2014 at 08:17:51AM -0500, Timur Tabi wrote:

Shengjiu Wang wrote:

quoted

+	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:

quoted

if (ssi_private->clk)

quoted

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 12:27:50PM +0100, Mark Brown wrote:

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

-	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:

quoted

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

-	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+	ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");

quoted

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 05:18:07PM +0800, Shengjiu Wang wrote:

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 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:

quoted

On Tue, Sep 09, 2014 at 12:27:50PM +0100, Mark Brown wrote:

quoted

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

-	ssi_private->clk = devm_clk_get(&pdev->dev, NULL);
+	ssi_private->clk = devm_clk_get(&pdev->dev, "ipg");

quoted

quoted

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

quoted

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.

quoted

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 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:

quoted

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:

quoted

+	/*
+	 * 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:

quoted

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 11:38:05AM -0700, Nicolin Chen wrote:

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

@@ -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

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

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:

quoted

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

@@ -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 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:

quoted

@@ -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 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:

quoted

On 09/09/2014 01:38 PM, Nicolin Chen wrote:

quoted

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 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:

quoted

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

@@ -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:

quoted

On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:

quoted

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:

quoted

@@ -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

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

On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:

quoted

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.

quoted

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:

quoted

quoted

Then we can get a patch like:
open() {
+	clk_prepare_enable();
	....
}

close() {
	....
+	clk_disable_unprepare()
}

quoted

what is the open() and close()? do you mean the fsl_ssi_startup()
and fsl_ssi_shutdown()?

Yea.

quoted

quoted

probe() {
	clk_get();
	clk_prepare_enable();
	....
	if (xxx)
-		goto err_xx;
+		return ret;
	....
+	clk_disable_unprepare();
	return 0;
-err_xx:
-	clk_disable_unprepare()
}

quoted

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.

quoted

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

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

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:

quoted

On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:

quoted

quoted

Then we can get a patch like:
open() {
+	clk_prepare_enable();
	....
}

close() {
	....
+	clk_disable_unprepare()
}

quoted

what is the open() and close()? do you mean the fsl_ssi_startup()
and fsl_ssi_shutdown()?

Yea.

quoted

quoted

probe() {
	clk_get();
	clk_prepare_enable();
	....
	if (xxx)
-		goto err_xx;
+		return ret;
	....
+	clk_disable_unprepare();
	return 0;
-err_xx:
-	clk_disable_unprepare()
}

quoted

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.

quoted

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.

quoted

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 |