[PATCH 2/6] mmc: sh_mmcif: do not manage PM clocks manually

[PATCH 2/6] mmc: sh_mmcif: do not manage PM clocks manually

[Guennadi Liakhovetski]

On sh-mobile platforms the MMC clock frequency for the MMCIF unit is
obtained from the same clock, as the one, that runtime power-manages the
controller. The MMCIF driver has to access that clock directly,
bypassing the runtime PM framework, to get its frequency, but it
shouldn't enable or disable it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mmcif.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 724b35e..b0bf19c 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -213,7 +213,6 @@ struct sh_mmcif_host {
 	struct platform_device *pd;
 	struct sh_dmae_slave dma_slave_tx;
 	struct sh_dmae_slave dma_slave_rx;
-	struct clk *hclk;
 	unsigned int clk;
 	int bus_width;
 	bool sd_error;
@@ -1249,6 +1248,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	struct sh_mmcif_host *host;
 	struct sh_mmcif_plat_data *pd;
 	struct resource *res;
+	struct clk *hclk;
 	void __iomem *reg;
 	char clk_name[8];
 
@@ -1285,14 +1285,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	host->timeout	= 1000;
 
 	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
-	host->hclk = clk_get(&pdev->dev, clk_name);
-	if (IS_ERR(host->hclk)) {
+	hclk = clk_get(&pdev->dev, clk_name);
+	if (IS_ERR(hclk)) {
 		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
-		ret = PTR_ERR(host->hclk);
+		ret = PTR_ERR(hclk);
 		goto clean_up1;
 	}
-	clk_enable(host->hclk);
-	host->clk = clk_get_rate(host->hclk);
+	host->clk = clk_get_rate(hclk);
+	clk_put(hclk);
 	host->pd = pdev;
 
 	spin_lock_init(&host->lock);
@@ -1355,7 +1355,6 @@ clean_up3:
 	pm_runtime_suspend(&pdev->dev);
 clean_up2:
 	pm_runtime_disable(&pdev->dev);
-	clk_disable(host->hclk);
 clean_up1:
 	mmc_free_host(mmc);
 clean_up:
@@ -1395,7 +1394,6 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	clk_disable(host->hclk);
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -1410,10 +1408,8 @@ static int sh_mmcif_suspend(struct device *dev)
 	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
 	int ret = mmc_suspend_host(host->mmc);
 
-	if (!ret) {
+	if (!ret)
 		sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
-		clk_disable(host->hclk);
-	}
 
 	return ret;
 }
@@ -1423,8 +1419,6 @@ static int sh_mmcif_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
 
-	clk_enable(host->hclk);
-
 	return mmc_resume_host(host->mmc);
 }
 #else
-- 
1.7.2.5

Re: [PATCH 2/6] mmc: sh_mmcif: do not manage PM clocks manually

[Paul Mundt]

On Wed, Apr 18, 2012 at 01:28:32PM +0200, Guennadi Liakhovetski wrote:
> On sh-mobile platforms the MMC clock frequency for the MMCIF unit is
> obtained from the same clock, as the one, that runtime power-manages the
> controller. The MMCIF driver has to access that clock directly,
> bypassing the runtime PM framework, to get its frequency, but it
> shouldn't enable or disable it.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

I don't understand your logic. clk_get_rate() requires the clock to be
enabled, the result is undefined otherwise. If you have a case where the
clock is already enabled and you can read the rate directly that's fine,
but it's still an API violation without enable/disable pairs.

Re: [PATCH 2/6] mmc: sh_mmcif: do not manage PM clocks manually

[Guennadi Liakhovetski]

On Wed, 18 Apr 2012, Paul Mundt wrote:

> On Wed, Apr 18, 2012 at 01:28:32PM +0200, Guennadi Liakhovetski wrote:
> > On sh-mobile platforms the MMC clock frequency for the MMCIF unit is
> > obtained from the same clock, as the one, that runtime power-manages the
> > controller. The MMCIF driver has to access that clock directly,
> > bypassing the runtime PM framework, to get its frequency, but it
> > shouldn't enable or disable it.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> I don't understand your logic.

The logic is simple: do not manage the same object (clk), using different 
APIs (runtime PM and clkdev).

> clk_get_rate() requires the clock to be
> enabled, the result is undefined otherwise. If you have a case where the
> clock is already enabled and you can read the rate directly that's fine,
> but it's still an API violation without enable/disable pairs.

Then the mmcif patch has to be updated. sh-mmcif is an SH-specific driver, 
and there we know, that on all platforms (so far), the runtime PM switches 
the same clock on and off for us, so, we can use this knowledge and just 
make sure to read out the clock frequency, when the device is awake. OTOH, 
as you say, this is not very clean from the API-conformance standpoint.

Another possibility would be to add a runtime PM call like 
pm_runtime_clk_get_rate(dev, con_id) to safely read out one of runtime PM 
clock's rate. Would this be a better option or should we stay generic and 
use the clkdev API to get the clock rate (turning it on before the reading 
and off afterwards) and the runtime PM API to turn it on and off as 
required for operation?

The MSIOF driver is ok as is - provided we accept, that the clock is 
turned on by runtime PM and its rate is read out using clkdev. The SDHI 
driver has to be fixed in one way or another.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/