[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates

[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates

[Guennadi Liakhovetski]

This patch series supersedes this thread

http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14626

and these patches:

http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14630
http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus=14638

It has been decided to reorganise the above series to split it into 
several smaller and simpler ones, to separate platform patches from driver 
patches to reduce cross-dependencies as much as possible.

It requires the recently posted v5 of "mmc: add a function to get 
regulators, supplying card's power," and patch "mmc: sh_mmcif: Support 
MMC_SLEEP_AWAKE command" from Laurent:

http://thread.gmane.org/gmane.linux.kernel.mmc/14939

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

[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates

[Guennadi Liakhovetski]

This patch series supersedes this thread

http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus\x14626

and these patches:

http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus\x14630
http://thread.gmane.org/gmane.linux.kernel.mmc/14624/focus\x14638

It has been decided to reorganise the above series to split it into 
several smaller and simpler ones, to separate platform patches from driver 
patches to reduce cross-dependencies as much as possible.

It requires the recently posted v5 of "mmc: add a function to get 
regulators, supplying card's power," and patch "mmc: sh_mmcif: Support 
MMC_SLEEP_AWAKE command" from Laurent:

http://thread.gmane.org/gmane.linux.kernel.mmc/14939

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

[PATCH 1/5 v4] mmc: sh_mmcif: simplify and use meaningful label names in error-handling

[Guennadi Liakhovetski]

A check for NULL platform data can be conveniently made in the very
beginning of probing. Replace numbered error-handling labels in .probe()
with meaningful names to make any future reorganisation simpler.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index e32da11..d6ffb05 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1241,11 +1241,16 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	int ret = 0, irq[2];
 	struct mmc_host *mmc;
 	struct sh_mmcif_host *host;
-	struct sh_mmcif_plat_data *pd;
+	struct sh_mmcif_plat_data *pd = pdev->dev.platform_data;
 	struct resource *res;
 	void __iomem *reg;
 	char clk_name[8];
 
+	if (!pd) {
+		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
+		return -ENXIO;
+	}
+
 	irq[0] = platform_get_irq(pdev, 0);
 	irq[1] = platform_get_irq(pdev, 1);
 	if (irq[0] < 0 || irq[1] < 0) {
@@ -1262,16 +1267,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "ioremap error.\n");
 		return -ENOMEM;
 	}
-	pd = pdev->dev.platform_data;
-	if (!pd) {
-		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
-		ret = -ENXIO;
-		goto clean_up;
-	}
+
 	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
 	if (!mmc) {
 		ret = -ENOMEM;
-		goto clean_up;
+		goto ealloch;
 	}
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
@@ -1283,7 +1283,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	if (IS_ERR(host->hclk)) {
 		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
 		ret = PTR_ERR(host->hclk);
-		goto clean_up1;
+		goto eclkget;
 	}
 	clk_enable(host->hclk);
 	host->clk = clk_get_rate(host->hclk);
@@ -1313,7 +1313,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
-		goto clean_up2;
+		goto eresume;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
@@ -1322,17 +1322,17 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (sh_mmc:error)\n");
-		goto clean_up3;
+		goto ereqirq0;
 	}
 	ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:int", host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-		goto clean_up4;
+		goto ereqirq1;
 	}
 
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
-		goto clean_up5;
+		goto emmcaddh;
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
@@ -1341,20 +1341,19 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
 
-clean_up5:
+emmcaddh:
 	free_irq(irq[1], host);
-clean_up4:
+ereqirq1:
 	free_irq(irq[0], host);
-clean_up3:
+ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
-clean_up2:
+eresume:
 	pm_runtime_disable(&pdev->dev);
 	clk_disable(host->hclk);
-clean_up1:
+eclkget:
 	mmc_free_host(mmc);
-clean_up:
-	if (reg)
-		iounmap(reg);
+ealloch:
+	iounmap(reg);
 	return ret;
 }
 
-- 
1.7.2.5

[PATCH 1/5 v4] mmc: sh_mmcif: simplify and use meaningful label names in error-handling

[Guennadi Liakhovetski]

A check for NULL platform data can be conveniently made in the very
beginning of probing. Replace numbered error-handling labels in .probe()
with meaningful names to make any future reorganisation simpler.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index e32da11..d6ffb05 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1241,11 +1241,16 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	int ret = 0, irq[2];
 	struct mmc_host *mmc;
 	struct sh_mmcif_host *host;
-	struct sh_mmcif_plat_data *pd;
+	struct sh_mmcif_plat_data *pd = pdev->dev.platform_data;
 	struct resource *res;
 	void __iomem *reg;
 	char clk_name[8];
 
+	if (!pd) {
+		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
+		return -ENXIO;
+	}
+
 	irq[0] = platform_get_irq(pdev, 0);
 	irq[1] = platform_get_irq(pdev, 1);
 	if (irq[0] < 0 || irq[1] < 0) {
@@ -1262,16 +1267,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "ioremap error.\n");
 		return -ENOMEM;
 	}
-	pd = pdev->dev.platform_data;
-	if (!pd) {
-		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
-		ret = -ENXIO;
-		goto clean_up;
-	}
+
 	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
 	if (!mmc) {
 		ret = -ENOMEM;
-		goto clean_up;
+		goto ealloch;
 	}
 	host		= mmc_priv(mmc);
 	host->mmc	= mmc;
@@ -1283,7 +1283,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	if (IS_ERR(host->hclk)) {
 		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
 		ret = PTR_ERR(host->hclk);
-		goto clean_up1;
+		goto eclkget;
 	}
 	clk_enable(host->hclk);
 	host->clk = clk_get_rate(host->hclk);
@@ -1313,7 +1313,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
-		goto clean_up2;
+		goto eresume;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
@@ -1322,17 +1322,17 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (sh_mmc:error)\n");
-		goto clean_up3;
+		goto ereqirq0;
 	}
 	ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:int", host);
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-		goto clean_up4;
+		goto ereqirq1;
 	}
 
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
-		goto clean_up5;
+		goto emmcaddh;
 
 	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
 
@@ -1341,20 +1341,19 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
 	return ret;
 
-clean_up5:
+emmcaddh:
 	free_irq(irq[1], host);
-clean_up4:
+ereqirq1:
 	free_irq(irq[0], host);
-clean_up3:
+ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
-clean_up2:
+eresume:
 	pm_runtime_disable(&pdev->dev);
 	clk_disable(host->hclk);
-clean_up1:
+eclkget:
 	mmc_free_host(mmc);
-clean_up:
-	if (reg)
-		iounmap(reg);
+ealloch:
+	iounmap(reg);
 	return ret;
 }
 
-- 
1.7.2.5

Re: [PATCH 1/5 v4] mmc: sh_mmcif: simplify and use meaningful label names in error-handling

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:15PM +0200, Guennadi Liakhovetski wrote:
> A check for NULL platform data can be conveniently made in the very
> beginning of probing. Replace numbered error-handling labels in .probe()
> with meaningful names to make any future reorganisation simpler.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

> ---
>  drivers/mmc/host/sh_mmcif.c |   41 ++++++++++++++++++++---------------------
>  1 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index e32da11..d6ffb05 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1241,11 +1241,16 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	int ret = 0, irq[2];
>  	struct mmc_host *mmc;
>  	struct sh_mmcif_host *host;
> -	struct sh_mmcif_plat_data *pd;
> +	struct sh_mmcif_plat_data *pd = pdev->dev.platform_data;
>  	struct resource *res;
>  	void __iomem *reg;
>  	char clk_name[8];
>  
> +	if (!pd) {
> +		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
> +		return -ENXIO;
> +	}
> +
>  	irq[0] = platform_get_irq(pdev, 0);
>  	irq[1] = platform_get_irq(pdev, 1);
>  	if (irq[0] < 0 || irq[1] < 0) {
> @@ -1262,16 +1267,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "ioremap error.\n");
>  		return -ENOMEM;
>  	}
> -	pd = pdev->dev.platform_data;
> -	if (!pd) {
> -		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
> -		ret = -ENXIO;
> -		goto clean_up;
> -	}
> +
>  	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
>  	if (!mmc) {
>  		ret = -ENOMEM;
> -		goto clean_up;
> +		goto ealloch;
>  	}
>  	host		= mmc_priv(mmc);
>  	host->mmc	= mmc;
> @@ -1283,7 +1283,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	if (IS_ERR(host->hclk)) {
>  		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
>  		ret = PTR_ERR(host->hclk);
> -		goto clean_up1;
> +		goto eclkget;
>  	}
>  	clk_enable(host->hclk);
>  	host->clk = clk_get_rate(host->hclk);
> @@ -1313,7 +1313,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  
>  	ret = pm_runtime_resume(&pdev->dev);
>  	if (ret < 0)
> -		goto clean_up2;
> +		goto eresume;
>  
>  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>  
> @@ -1322,17 +1322,17 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
>  	if (ret) {
>  		dev_err(&pdev->dev, "request_irq error (sh_mmc:error)\n");
> -		goto clean_up3;
> +		goto ereqirq0;
>  	}
>  	ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:int", host);
>  	if (ret) {
>  		dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
> -		goto clean_up4;
> +		goto ereqirq1;
>  	}
>  
>  	ret = mmc_add_host(mmc);
>  	if (ret < 0)
> -		goto clean_up5;
> +		goto emmcaddh;
>  
>  	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
>  
> @@ -1341,20 +1341,19 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>  	return ret;
>  
> -clean_up5:
> +emmcaddh:
>  	free_irq(irq[1], host);
> -clean_up4:
> +ereqirq1:
>  	free_irq(irq[0], host);
> -clean_up3:
> +ereqirq0:
>  	pm_runtime_suspend(&pdev->dev);
> -clean_up2:
> +eresume:
>  	pm_runtime_disable(&pdev->dev);
>  	clk_disable(host->hclk);
> -clean_up1:
> +eclkget:
>  	mmc_free_host(mmc);
> -clean_up:
> -	if (reg)
> -		iounmap(reg);

It looks like this if (reg) was redundant: reg was always non-NULL if
this path is executed. Good riddance :)

> +ealloch:
> +	iounmap(reg);
>  	return ret;
>  }

Re: [PATCH 1/5 v4] mmc: sh_mmcif: simplify and use meaningful label names in error-handling

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:15PM +0200, Guennadi Liakhovetski wrote:
> A check for NULL platform data can be conveniently made in the very
> beginning of probing. Replace numbered error-handling labels in .probe()
> with meaningful names to make any future reorganisation simpler.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

> ---
>  drivers/mmc/host/sh_mmcif.c |   41 ++++++++++++++++++++---------------------
>  1 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index e32da11..d6ffb05 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1241,11 +1241,16 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	int ret = 0, irq[2];
>  	struct mmc_host *mmc;
>  	struct sh_mmcif_host *host;
> -	struct sh_mmcif_plat_data *pd;
> +	struct sh_mmcif_plat_data *pd = pdev->dev.platform_data;
>  	struct resource *res;
>  	void __iomem *reg;
>  	char clk_name[8];
>  
> +	if (!pd) {
> +		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
> +		return -ENXIO;
> +	}
> +
>  	irq[0] = platform_get_irq(pdev, 0);
>  	irq[1] = platform_get_irq(pdev, 1);
>  	if (irq[0] < 0 || irq[1] < 0) {
> @@ -1262,16 +1267,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "ioremap error.\n");
>  		return -ENOMEM;
>  	}
> -	pd = pdev->dev.platform_data;
> -	if (!pd) {
> -		dev_err(&pdev->dev, "sh_mmcif plat data error.\n");
> -		ret = -ENXIO;
> -		goto clean_up;
> -	}
> +
>  	mmc = mmc_alloc_host(sizeof(struct sh_mmcif_host), &pdev->dev);
>  	if (!mmc) {
>  		ret = -ENOMEM;
> -		goto clean_up;
> +		goto ealloch;
>  	}
>  	host		= mmc_priv(mmc);
>  	host->mmc	= mmc;
> @@ -1283,7 +1283,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	if (IS_ERR(host->hclk)) {
>  		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
>  		ret = PTR_ERR(host->hclk);
> -		goto clean_up1;
> +		goto eclkget;
>  	}
>  	clk_enable(host->hclk);
>  	host->clk = clk_get_rate(host->hclk);
> @@ -1313,7 +1313,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  
>  	ret = pm_runtime_resume(&pdev->dev);
>  	if (ret < 0)
> -		goto clean_up2;
> +		goto eresume;
>  
>  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>  
> @@ -1322,17 +1322,17 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
>  	if (ret) {
>  		dev_err(&pdev->dev, "request_irq error (sh_mmc:error)\n");
> -		goto clean_up3;
> +		goto ereqirq0;
>  	}
>  	ret = request_threaded_irq(irq[1], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:int", host);
>  	if (ret) {
>  		dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
> -		goto clean_up4;
> +		goto ereqirq1;
>  	}
>  
>  	ret = mmc_add_host(mmc);
>  	if (ret < 0)
> -		goto clean_up5;
> +		goto emmcaddh;
>  
>  	dev_pm_qos_expose_latency_limit(&pdev->dev, 100);
>  
> @@ -1341,20 +1341,19 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x0000ffff);
>  	return ret;
>  
> -clean_up5:
> +emmcaddh:
>  	free_irq(irq[1], host);
> -clean_up4:
> +ereqirq1:
>  	free_irq(irq[0], host);
> -clean_up3:
> +ereqirq0:
>  	pm_runtime_suspend(&pdev->dev);
> -clean_up2:
> +eresume:
>  	pm_runtime_disable(&pdev->dev);
>  	clk_disable(host->hclk);
> -clean_up1:
> +eclkget:
>  	mmc_free_host(mmc);
> -clean_up:
> -	if (reg)
> -		iounmap(reg);

It looks like this if (reg) was redundant: reg was always non-NULL if
this path is executed. Good riddance :)

> +ealloch:
> +	iounmap(reg);
>  	return ret;
>  }

[PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Guennadi Liakhovetski]

Regardless, whether the MMC bus clock is the same, as the PM clock on this
specific interface, it has to be managed separately. Its proper management
should also include enabling and disabling of the clock, whenever the
interface is becoming active or going idle respectively.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index d6ffb05..6a93b04 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		}
 		if (host->power) {
 			pm_runtime_put(&host->pd->dev);
+			clk_disable(host->hclk);
 			host->power = false;
 			if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
 				p->down_pwr(host->pd);
@@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (!host->power) {
 			if (p->set_pwr)
 				p->set_pwr(host->pd, ios->power_mode);
+			clk_enable(host->hclk);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
 			sh_mmcif_sync_reset(host);
@@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	host->addr	= reg;
 	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)) {
-		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
-		ret = PTR_ERR(host->hclk);
-		goto eclkget;
-	}
-	clk_enable(host->hclk);
-	host->clk = clk_get_rate(host->hclk);
 	host->pd = pdev;
 
 	spin_lock_init(&host->lock);
 
 	mmc->ops = &sh_mmcif_ops;
-	mmc->f_max = host->clk / 2;
-	mmc->f_min = host->clk / 512;
 	if (pd->ocr)
 		mmc->ocr_avail = pd->ocr;
 	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
@@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
 	mmc->max_seg_size = mmc->max_req_size;
 
-	sh_mmcif_sync_reset(host);
 	platform_set_drvdata(pdev, host);
 
 	pm_runtime_enable(&pdev->dev);
 	host->power = false;
 
+	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
+	host->hclk = clk_get(&pdev->dev, clk_name);
+	if (IS_ERR(host->hclk)) {
+		ret = PTR_ERR(host->hclk);
+		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
+		goto eclkget;
+	}
+	clk_enable(host->hclk);
+	host->clk = clk_get_rate(host->hclk);
+	mmc->f_max = host->clk / 2;
+	mmc->f_min = host->clk / 512;
+
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
 		goto eresume;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
+	sh_mmcif_sync_reset(host);
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 
 	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
@@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		goto ereqirq1;
 	}
 
+	clk_disable(host->hclk);
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
 		goto emmcaddh;
@@ -1348,9 +1352,10 @@ ereqirq1:
 ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
 eresume:
-	pm_runtime_disable(&pdev->dev);
 	clk_disable(host->hclk);
+	clk_put(host->hclk);
 eclkget:
+	pm_runtime_disable(&pdev->dev);
 	mmc_free_host(mmc);
 ealloch:
 	iounmap(reg);
@@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 	int irq[2];
 
 	host->dying = true;
+	clk_enable(host->hclk);
 	pm_runtime_get_sync(&pdev->dev);
 
 	dev_pm_qos_hide_latency_limit(&pdev->dev);
@@ -1388,9 +1394,9 @@ 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);
+	clk_disable(host->hclk);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
@@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int sh_mmcif_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
+	struct sh_mmcif_host *host = dev_get_drvdata(dev);
 	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;
 }
 
 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);
+	struct sh_mmcif_host *host = dev_get_drvdata(dev);
 
 	return mmc_resume_host(host->mmc);
 }
-- 
1.7.2.5

[PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Guennadi Liakhovetski]

Regardless, whether the MMC bus clock is the same, as the PM clock on this
specific interface, it has to be managed separately. Its proper management
should also include enabling and disabling of the clock, whenever the
interface is becoming active or going idle respectively.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index d6ffb05..6a93b04 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		}
 		if (host->power) {
 			pm_runtime_put(&host->pd->dev);
+			clk_disable(host->hclk);
 			host->power = false;
 			if (p->down_pwr && ios->power_mode = MMC_POWER_OFF)
 				p->down_pwr(host->pd);
@@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (!host->power) {
 			if (p->set_pwr)
 				p->set_pwr(host->pd, ios->power_mode);
+			clk_enable(host->hclk);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
 			sh_mmcif_sync_reset(host);
@@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	host->addr	= reg;
 	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)) {
-		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
-		ret = PTR_ERR(host->hclk);
-		goto eclkget;
-	}
-	clk_enable(host->hclk);
-	host->clk = clk_get_rate(host->hclk);
 	host->pd = pdev;
 
 	spin_lock_init(&host->lock);
 
 	mmc->ops = &sh_mmcif_ops;
-	mmc->f_max = host->clk / 2;
-	mmc->f_min = host->clk / 512;
 	if (pd->ocr)
 		mmc->ocr_avail = pd->ocr;
 	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
@@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
 	mmc->max_seg_size = mmc->max_req_size;
 
-	sh_mmcif_sync_reset(host);
 	platform_set_drvdata(pdev, host);
 
 	pm_runtime_enable(&pdev->dev);
 	host->power = false;
 
+	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
+	host->hclk = clk_get(&pdev->dev, clk_name);
+	if (IS_ERR(host->hclk)) {
+		ret = PTR_ERR(host->hclk);
+		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
+		goto eclkget;
+	}
+	clk_enable(host->hclk);
+	host->clk = clk_get_rate(host->hclk);
+	mmc->f_max = host->clk / 2;
+	mmc->f_min = host->clk / 512;
+
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
 		goto eresume;
 
 	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
 
+	sh_mmcif_sync_reset(host);
 	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
 
 	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
@@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		goto ereqirq1;
 	}
 
+	clk_disable(host->hclk);
 	ret = mmc_add_host(mmc);
 	if (ret < 0)
 		goto emmcaddh;
@@ -1348,9 +1352,10 @@ ereqirq1:
 ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
 eresume:
-	pm_runtime_disable(&pdev->dev);
 	clk_disable(host->hclk);
+	clk_put(host->hclk);
 eclkget:
+	pm_runtime_disable(&pdev->dev);
 	mmc_free_host(mmc);
 ealloch:
 	iounmap(reg);
@@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 	int irq[2];
 
 	host->dying = true;
+	clk_enable(host->hclk);
 	pm_runtime_get_sync(&pdev->dev);
 
 	dev_pm_qos_hide_latency_limit(&pdev->dev);
@@ -1388,9 +1394,9 @@ 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);
+	clk_disable(host->hclk);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
@@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int sh_mmcif_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
+	struct sh_mmcif_host *host = dev_get_drvdata(dev);
 	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;
 }
 
 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);
+	struct sh_mmcif_host *host = dev_get_drvdata(dev);
 
 	return mmc_resume_host(host->mmc);
 }
-- 
1.7.2.5

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Simon Horman]

Hi Guennadi,

On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> Regardless, whether the MMC bus clock is the same, as the PM clock on this
> specific interface, it has to be managed separately. Its proper management
> should also include enabling and disabling of the clock, whenever the
> interface is becoming active or going idle respectively.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
>  1 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index d6ffb05..6a93b04 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		}
>  		if (host->power) {
>  			pm_runtime_put(&host->pd->dev);
> +			clk_disable(host->hclk);
>  			host->power = false;
>  			if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
>  				p->down_pwr(host->pd);
> @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		if (!host->power) {
>  			if (p->set_pwr)
>  				p->set_pwr(host->pd, ios->power_mode);
> +			clk_enable(host->hclk);
>  			pm_runtime_get_sync(&host->pd->dev);
>  			host->power = true;
>  			sh_mmcif_sync_reset(host);
> @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	host->addr	= reg;
>  	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)) {
> -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> -		ret = PTR_ERR(host->hclk);
> -		goto eclkget;
> -	}
> -	clk_enable(host->hclk);
> -	host->clk = clk_get_rate(host->hclk);
>  	host->pd = pdev;
>  
>  	spin_lock_init(&host->lock);
>  
>  	mmc->ops = &sh_mmcif_ops;
> -	mmc->f_max = host->clk / 2;
> -	mmc->f_min = host->clk / 512;
>  	if (pd->ocr)
>  		mmc->ocr_avail = pd->ocr;
>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
>  	mmc->max_seg_size = mmc->max_req_size;
>  
> -	sh_mmcif_sync_reset(host);
>  	platform_set_drvdata(pdev, host);
>  
>  	pm_runtime_enable(&pdev->dev);
>  	host->power = false;
>  
> +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> +	host->hclk = clk_get(&pdev->dev, clk_name);
> +	if (IS_ERR(host->hclk)) {
> +		ret = PTR_ERR(host->hclk);
> +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> +		goto eclkget;
> +	}
> +	clk_enable(host->hclk);
> +	host->clk = clk_get_rate(host->hclk);
> +	mmc->f_max = host->clk / 2;
> +	mmc->f_min = host->clk / 512;
> +
>  	ret = pm_runtime_resume(&pdev->dev);
>  	if (ret < 0)
>  		goto eresume;
>  
>  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>  
> +	sh_mmcif_sync_reset(host);
>  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
>  
>  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		goto ereqirq1;
>  	}
>  
> +	clk_disable(host->hclk);
>  	ret = mmc_add_host(mmc);
>  	if (ret < 0)
>  		goto emmcaddh;

Could you clarify the purpose of two hunks above.
They seem to move code down in sh_mmcif_probe().
Is this related to the call to pm_runtime_enable() ?

> @@ -1348,9 +1352,10 @@ ereqirq1:
>  ereqirq0:
>  	pm_runtime_suspend(&pdev->dev);
>  eresume:
> -	pm_runtime_disable(&pdev->dev);
>  	clk_disable(host->hclk);
> +	clk_put(host->hclk);
>  eclkget:
> +	pm_runtime_disable(&pdev->dev);
>  	mmc_free_host(mmc);
>  ealloch:
>  	iounmap(reg);
> @@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  	int irq[2];
>  
>  	host->dying = true;
> +	clk_enable(host->hclk);
>  	pm_runtime_get_sync(&pdev->dev);
>  
>  	dev_pm_qos_hide_latency_limit(&pdev->dev);
> @@ -1388,9 +1394,9 @@ 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);
> +	clk_disable(host->hclk);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	return 0;
> @@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int sh_mmcif_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
> +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
>  	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;
>  }
>  
>  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);
> +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
>  
>  	return mmc_resume_host(host->mmc);
>  }
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Simon Horman]

Hi Guennadi,

On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> Regardless, whether the MMC bus clock is the same, as the PM clock on this
> specific interface, it has to be managed separately. Its proper management
> should also include enabling and disabling of the clock, whenever the
> interface is becoming active or going idle respectively.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
>  1 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index d6ffb05..6a93b04 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		}
>  		if (host->power) {
>  			pm_runtime_put(&host->pd->dev);
> +			clk_disable(host->hclk);
>  			host->power = false;
>  			if (p->down_pwr && ios->power_mode = MMC_POWER_OFF)
>  				p->down_pwr(host->pd);
> @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		if (!host->power) {
>  			if (p->set_pwr)
>  				p->set_pwr(host->pd, ios->power_mode);
> +			clk_enable(host->hclk);
>  			pm_runtime_get_sync(&host->pd->dev);
>  			host->power = true;
>  			sh_mmcif_sync_reset(host);
> @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	host->addr	= reg;
>  	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)) {
> -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> -		ret = PTR_ERR(host->hclk);
> -		goto eclkget;
> -	}
> -	clk_enable(host->hclk);
> -	host->clk = clk_get_rate(host->hclk);
>  	host->pd = pdev;
>  
>  	spin_lock_init(&host->lock);
>  
>  	mmc->ops = &sh_mmcif_ops;
> -	mmc->f_max = host->clk / 2;
> -	mmc->f_min = host->clk / 512;
>  	if (pd->ocr)
>  		mmc->ocr_avail = pd->ocr;
>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
>  	mmc->max_seg_size = mmc->max_req_size;
>  
> -	sh_mmcif_sync_reset(host);
>  	platform_set_drvdata(pdev, host);
>  
>  	pm_runtime_enable(&pdev->dev);
>  	host->power = false;
>  
> +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> +	host->hclk = clk_get(&pdev->dev, clk_name);
> +	if (IS_ERR(host->hclk)) {
> +		ret = PTR_ERR(host->hclk);
> +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> +		goto eclkget;
> +	}
> +	clk_enable(host->hclk);
> +	host->clk = clk_get_rate(host->hclk);
> +	mmc->f_max = host->clk / 2;
> +	mmc->f_min = host->clk / 512;
> +
>  	ret = pm_runtime_resume(&pdev->dev);
>  	if (ret < 0)
>  		goto eresume;
>  
>  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
>  
> +	sh_mmcif_sync_reset(host);
>  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
>  
>  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		goto ereqirq1;
>  	}
>  
> +	clk_disable(host->hclk);
>  	ret = mmc_add_host(mmc);
>  	if (ret < 0)
>  		goto emmcaddh;

Could you clarify the purpose of two hunks above.
They seem to move code down in sh_mmcif_probe().
Is this related to the call to pm_runtime_enable() ?

> @@ -1348,9 +1352,10 @@ ereqirq1:
>  ereqirq0:
>  	pm_runtime_suspend(&pdev->dev);
>  eresume:
> -	pm_runtime_disable(&pdev->dev);
>  	clk_disable(host->hclk);
> +	clk_put(host->hclk);
>  eclkget:
> +	pm_runtime_disable(&pdev->dev);
>  	mmc_free_host(mmc);
>  ealloch:
>  	iounmap(reg);
> @@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  	int irq[2];
>  
>  	host->dying = true;
> +	clk_enable(host->hclk);
>  	pm_runtime_get_sync(&pdev->dev);
>  
>  	dev_pm_qos_hide_latency_limit(&pdev->dev);
> @@ -1388,9 +1394,9 @@ 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);
> +	clk_disable(host->hclk);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	return 0;
> @@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  #ifdef CONFIG_PM
>  static int sh_mmcif_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
> +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
>  	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;
>  }
>  
>  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);
> +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
>  
>  	return mmc_resume_host(host->mmc);
>  }
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Guennadi Liakhovetski]

On Wed, 20 Jun 2012, Simon Horman wrote:

> Hi Guennadi,
> 
> On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> > Regardless, whether the MMC bus clock is the same, as the PM clock on this
> > specific interface, it has to be managed separately. Its proper management
> > should also include enabling and disabling of the clock, whenever the
> > interface is becoming active or going idle respectively.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
> >  1 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > index d6ffb05..6a93b04 100644
> > --- a/drivers/mmc/host/sh_mmcif.c
> > +++ b/drivers/mmc/host/sh_mmcif.c
> > @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  		}
> >  		if (host->power) {
> >  			pm_runtime_put(&host->pd->dev);
> > +			clk_disable(host->hclk);
> >  			host->power = false;
> >  			if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
> >  				p->down_pwr(host->pd);
> > @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  		if (!host->power) {
> >  			if (p->set_pwr)
> >  				p->set_pwr(host->pd, ios->power_mode);
> > +			clk_enable(host->hclk);
> >  			pm_runtime_get_sync(&host->pd->dev);
> >  			host->power = true;
> >  			sh_mmcif_sync_reset(host);
> > @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	host->addr	= reg;
> >  	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)) {
> > -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> > -		ret = PTR_ERR(host->hclk);
> > -		goto eclkget;
> > -	}
> > -	clk_enable(host->hclk);
> > -	host->clk = clk_get_rate(host->hclk);
> >  	host->pd = pdev;
> >  
> >  	spin_lock_init(&host->lock);
> >  
> >  	mmc->ops = &sh_mmcif_ops;
> > -	mmc->f_max = host->clk / 2;
> > -	mmc->f_min = host->clk / 512;
> >  	if (pd->ocr)
> >  		mmc->ocr_avail = pd->ocr;
> >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> > @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
> >  	mmc->max_seg_size = mmc->max_req_size;
> >  
> > -	sh_mmcif_sync_reset(host);
> >  	platform_set_drvdata(pdev, host);
> >  
> >  	pm_runtime_enable(&pdev->dev);
> >  	host->power = false;
> >  
> > +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > +	host->hclk = clk_get(&pdev->dev, clk_name);
> > +	if (IS_ERR(host->hclk)) {
> > +		ret = PTR_ERR(host->hclk);
> > +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> > +		goto eclkget;
> > +	}
> > +	clk_enable(host->hclk);
> > +	host->clk = clk_get_rate(host->hclk);
> > +	mmc->f_max = host->clk / 2;
> > +	mmc->f_min = host->clk / 512;
> > +
> >  	ret = pm_runtime_resume(&pdev->dev);
> >  	if (ret < 0)
> >  		goto eresume;
> >  
> >  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
> >  
> > +	sh_mmcif_sync_reset(host);
> >  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
> >  
> >  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> > @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  		goto ereqirq1;
> >  	}
> >  
> > +	clk_disable(host->hclk);
> >  	ret = mmc_add_host(mmc);
> >  	if (ret < 0)
> >  		goto emmcaddh;
> 
> Could you clarify the purpose of two hunks above.
> They seem to move code down in sh_mmcif_probe().
> Is this related to the call to pm_runtime_enable() ?

The sh_mmcif_sync_reset() call accesses the hardware so it only makes 
sense after the hardware has been resumed. Moving the clk_enable() call 
down improves consistency by grouping clock and runtime PM code together 
during probe() similar to other paths.

Thanks
Guennadi

> 
> > @@ -1348,9 +1352,10 @@ ereqirq1:
> >  ereqirq0:
> >  	pm_runtime_suspend(&pdev->dev);
> >  eresume:
> > -	pm_runtime_disable(&pdev->dev);
> >  	clk_disable(host->hclk);
> > +	clk_put(host->hclk);
> >  eclkget:
> > +	pm_runtime_disable(&pdev->dev);
> >  	mmc_free_host(mmc);
> >  ealloch:
> >  	iounmap(reg);
> > @@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
> >  	int irq[2];
> >  
> >  	host->dying = true;
> > +	clk_enable(host->hclk);
> >  	pm_runtime_get_sync(&pdev->dev);
> >  
> >  	dev_pm_qos_hide_latency_limit(&pdev->dev);
> > @@ -1388,9 +1394,9 @@ 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);
> > +	clk_disable(host->hclk);
> >  	pm_runtime_disable(&pdev->dev);
> >  
> >  	return 0;
> > @@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
> >  #ifdef CONFIG_PM
> >  static int sh_mmcif_suspend(struct device *dev)
> >  {
> > -	struct platform_device *pdev = to_platform_device(dev);
> > -	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
> > +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
> >  	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;
> >  }
> >  
> >  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);
> > +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
> >  
> >  	return mmc_resume_host(host->mmc);
> >  }
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

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

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Guennadi Liakhovetski]

On Wed, 20 Jun 2012, Simon Horman wrote:

> Hi Guennadi,
> 
> On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> > Regardless, whether the MMC bus clock is the same, as the PM clock on this
> > specific interface, it has to be managed separately. Its proper management
> > should also include enabling and disabling of the clock, whenever the
> > interface is becoming active or going idle respectively.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
> >  1 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > index d6ffb05..6a93b04 100644
> > --- a/drivers/mmc/host/sh_mmcif.c
> > +++ b/drivers/mmc/host/sh_mmcif.c
> > @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  		}
> >  		if (host->power) {
> >  			pm_runtime_put(&host->pd->dev);
> > +			clk_disable(host->hclk);
> >  			host->power = false;
> >  			if (p->down_pwr && ios->power_mode = MMC_POWER_OFF)
> >  				p->down_pwr(host->pd);
> > @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  		if (!host->power) {
> >  			if (p->set_pwr)
> >  				p->set_pwr(host->pd, ios->power_mode);
> > +			clk_enable(host->hclk);
> >  			pm_runtime_get_sync(&host->pd->dev);
> >  			host->power = true;
> >  			sh_mmcif_sync_reset(host);
> > @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	host->addr	= reg;
> >  	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)) {
> > -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> > -		ret = PTR_ERR(host->hclk);
> > -		goto eclkget;
> > -	}
> > -	clk_enable(host->hclk);
> > -	host->clk = clk_get_rate(host->hclk);
> >  	host->pd = pdev;
> >  
> >  	spin_lock_init(&host->lock);
> >  
> >  	mmc->ops = &sh_mmcif_ops;
> > -	mmc->f_max = host->clk / 2;
> > -	mmc->f_min = host->clk / 512;
> >  	if (pd->ocr)
> >  		mmc->ocr_avail = pd->ocr;
> >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> > @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
> >  	mmc->max_seg_size = mmc->max_req_size;
> >  
> > -	sh_mmcif_sync_reset(host);
> >  	platform_set_drvdata(pdev, host);
> >  
> >  	pm_runtime_enable(&pdev->dev);
> >  	host->power = false;
> >  
> > +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > +	host->hclk = clk_get(&pdev->dev, clk_name);
> > +	if (IS_ERR(host->hclk)) {
> > +		ret = PTR_ERR(host->hclk);
> > +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> > +		goto eclkget;
> > +	}
> > +	clk_enable(host->hclk);
> > +	host->clk = clk_get_rate(host->hclk);
> > +	mmc->f_max = host->clk / 2;
> > +	mmc->f_min = host->clk / 512;
> > +
> >  	ret = pm_runtime_resume(&pdev->dev);
> >  	if (ret < 0)
> >  		goto eresume;
> >  
> >  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
> >  
> > +	sh_mmcif_sync_reset(host);
> >  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
> >  
> >  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> > @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  		goto ereqirq1;
> >  	}
> >  
> > +	clk_disable(host->hclk);
> >  	ret = mmc_add_host(mmc);
> >  	if (ret < 0)
> >  		goto emmcaddh;
> 
> Could you clarify the purpose of two hunks above.
> They seem to move code down in sh_mmcif_probe().
> Is this related to the call to pm_runtime_enable() ?

The sh_mmcif_sync_reset() call accesses the hardware so it only makes 
sense after the hardware has been resumed. Moving the clk_enable() call 
down improves consistency by grouping clock and runtime PM code together 
during probe() similar to other paths.

Thanks
Guennadi

> 
> > @@ -1348,9 +1352,10 @@ ereqirq1:
> >  ereqirq0:
> >  	pm_runtime_suspend(&pdev->dev);
> >  eresume:
> > -	pm_runtime_disable(&pdev->dev);
> >  	clk_disable(host->hclk);
> > +	clk_put(host->hclk);
> >  eclkget:
> > +	pm_runtime_disable(&pdev->dev);
> >  	mmc_free_host(mmc);
> >  ealloch:
> >  	iounmap(reg);
> > @@ -1363,6 +1368,7 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
> >  	int irq[2];
> >  
> >  	host->dying = true;
> > +	clk_enable(host->hclk);
> >  	pm_runtime_get_sync(&pdev->dev);
> >  
> >  	dev_pm_qos_hide_latency_limit(&pdev->dev);
> > @@ -1388,9 +1394,9 @@ 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);
> > +	clk_disable(host->hclk);
> >  	pm_runtime_disable(&pdev->dev);
> >  
> >  	return 0;
> > @@ -1399,24 +1405,18 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
> >  #ifdef CONFIG_PM
> >  static int sh_mmcif_suspend(struct device *dev)
> >  {
> > -	struct platform_device *pdev = to_platform_device(dev);
> > -	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
> > +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
> >  	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;
> >  }
> >  
> >  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);
> > +	struct sh_mmcif_host *host = dev_get_drvdata(dev);
> >  
> >  	return mmc_resume_host(host->mmc);
> >  }
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

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

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Simon Horman]

On Wed, Jun 20, 2012 at 08:41:42AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > Hi Guennadi,
> > 
> > On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> > > Regardless, whether the MMC bus clock is the same, as the PM clock on this
> > > specific interface, it has to be managed separately. Its proper management
> > > should also include enabling and disabling of the clock, whenever the
> > > interface is becoming active or going idle respectively.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
> > >  1 files changed, 23 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > > index d6ffb05..6a93b04 100644
> > > --- a/drivers/mmc/host/sh_mmcif.c
> > > +++ b/drivers/mmc/host/sh_mmcif.c
> > > @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  		}
> > >  		if (host->power) {
> > >  			pm_runtime_put(&host->pd->dev);
> > > +			clk_disable(host->hclk);
> > >  			host->power = false;
> > >  			if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
> > >  				p->down_pwr(host->pd);
> > > @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  		if (!host->power) {
> > >  			if (p->set_pwr)
> > >  				p->set_pwr(host->pd, ios->power_mode);
> > > +			clk_enable(host->hclk);
> > >  			pm_runtime_get_sync(&host->pd->dev);
> > >  			host->power = true;
> > >  			sh_mmcif_sync_reset(host);
> > > @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  	host->addr	= reg;
> > >  	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)) {
> > > -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> > > -		ret = PTR_ERR(host->hclk);
> > > -		goto eclkget;
> > > -	}
> > > -	clk_enable(host->hclk);
> > > -	host->clk = clk_get_rate(host->hclk);
> > >  	host->pd = pdev;
> > >  
> > >  	spin_lock_init(&host->lock);
> > >  
> > >  	mmc->ops = &sh_mmcif_ops;
> > > -	mmc->f_max = host->clk / 2;
> > > -	mmc->f_min = host->clk / 512;
> > >  	if (pd->ocr)
> > >  		mmc->ocr_avail = pd->ocr;
> > >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> > > @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
> > >  	mmc->max_seg_size = mmc->max_req_size;
> > >  
> > > -	sh_mmcif_sync_reset(host);
> > >  	platform_set_drvdata(pdev, host);
> > >  
> > >  	pm_runtime_enable(&pdev->dev);
> > >  	host->power = false;
> > >  
> > > +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > > +	host->hclk = clk_get(&pdev->dev, clk_name);
> > > +	if (IS_ERR(host->hclk)) {
> > > +		ret = PTR_ERR(host->hclk);
> > > +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> > > +		goto eclkget;
> > > +	}
> > > +	clk_enable(host->hclk);
> > > +	host->clk = clk_get_rate(host->hclk);
> > > +	mmc->f_max = host->clk / 2;
> > > +	mmc->f_min = host->clk / 512;
> > > +
> > >  	ret = pm_runtime_resume(&pdev->dev);
> > >  	if (ret < 0)
> > >  		goto eresume;
> > >  
> > >  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
> > >  
> > > +	sh_mmcif_sync_reset(host);
> > >  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
> > >  
> > >  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> > > @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  		goto ereqirq1;
> > >  	}
> > >  
> > > +	clk_disable(host->hclk);
> > >  	ret = mmc_add_host(mmc);
> > >  	if (ret < 0)
> > >  		goto emmcaddh;
> > 
> > Could you clarify the purpose of two hunks above.
> > They seem to move code down in sh_mmcif_probe().
> > Is this related to the call to pm_runtime_enable() ?
> 
> The sh_mmcif_sync_reset() call accesses the hardware so it only makes 
> sense after the hardware has been resumed. Moving the clk_enable() call 
> down improves consistency by grouping clock and runtime PM code together 
> during probe() similar to other paths.

Thanks.

Reviewed-by: Simon Horman <horms@verge.net.au>

Re: [PATCH 2/5 v4] mmc: sh_mmcif: fix clock management

[Simon Horman]

On Wed, Jun 20, 2012 at 08:41:42AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > Hi Guennadi,
> > 
> > On Wed, Jun 13, 2012 at 03:37:19PM +0200, Guennadi Liakhovetski wrote:
> > > Regardless, whether the MMC bus clock is the same, as the PM clock on this
> > > specific interface, it has to be managed separately. Its proper management
> > > should also include enabling and disabling of the clock, whenever the
> > > interface is becoming active or going idle respectively.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >  drivers/mmc/host/sh_mmcif.c |   46 +++++++++++++++++++++---------------------
> > >  1 files changed, 23 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > > index d6ffb05..6a93b04 100644
> > > --- a/drivers/mmc/host/sh_mmcif.c
> > > +++ b/drivers/mmc/host/sh_mmcif.c
> > > @@ -942,6 +942,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  		}
> > >  		if (host->power) {
> > >  			pm_runtime_put(&host->pd->dev);
> > > +			clk_disable(host->hclk);
> > >  			host->power = false;
> > >  			if (p->down_pwr && ios->power_mode = MMC_POWER_OFF)
> > >  				p->down_pwr(host->pd);
> > > @@ -954,6 +955,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  		if (!host->power) {
> > >  			if (p->set_pwr)
> > >  				p->set_pwr(host->pd, ios->power_mode);
> > > +			clk_enable(host->hclk);
> > >  			pm_runtime_get_sync(&host->pd->dev);
> > >  			host->power = true;
> > >  			sh_mmcif_sync_reset(host);
> > > @@ -1278,22 +1280,11 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  	host->addr	= reg;
> > >  	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)) {
> > > -		dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
> > > -		ret = PTR_ERR(host->hclk);
> > > -		goto eclkget;
> > > -	}
> > > -	clk_enable(host->hclk);
> > > -	host->clk = clk_get_rate(host->hclk);
> > >  	host->pd = pdev;
> > >  
> > >  	spin_lock_init(&host->lock);
> > >  
> > >  	mmc->ops = &sh_mmcif_ops;
> > > -	mmc->f_max = host->clk / 2;
> > > -	mmc->f_min = host->clk / 512;
> > >  	if (pd->ocr)
> > >  		mmc->ocr_avail = pd->ocr;
> > >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> > > @@ -1305,18 +1296,30 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
> > >  	mmc->max_seg_size = mmc->max_req_size;
> > >  
> > > -	sh_mmcif_sync_reset(host);
> > >  	platform_set_drvdata(pdev, host);
> > >  
> > >  	pm_runtime_enable(&pdev->dev);
> > >  	host->power = false;
> > >  
> > > +	snprintf(clk_name, sizeof(clk_name), "mmc%d", pdev->id);
> > > +	host->hclk = clk_get(&pdev->dev, clk_name);
> > > +	if (IS_ERR(host->hclk)) {
> > > +		ret = PTR_ERR(host->hclk);
> > > +		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
> > > +		goto eclkget;
> > > +	}
> > > +	clk_enable(host->hclk);
> > > +	host->clk = clk_get_rate(host->hclk);
> > > +	mmc->f_max = host->clk / 2;
> > > +	mmc->f_min = host->clk / 512;
> > > +
> > >  	ret = pm_runtime_resume(&pdev->dev);
> > >  	if (ret < 0)
> > >  		goto eresume;
> > >  
> > >  	INIT_DELAYED_WORK(&host->timeout_work, mmcif_timeout_work);
> > >  
> > > +	sh_mmcif_sync_reset(host);
> > >  	sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
> > >  
> > >  	ret = request_threaded_irq(irq[0], sh_mmcif_intr, sh_mmcif_irqt, 0, "sh_mmc:error", host);
> > > @@ -1330,6 +1333,7 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> > >  		goto ereqirq1;
> > >  	}
> > >  
> > > +	clk_disable(host->hclk);
> > >  	ret = mmc_add_host(mmc);
> > >  	if (ret < 0)
> > >  		goto emmcaddh;
> > 
> > Could you clarify the purpose of two hunks above.
> > They seem to move code down in sh_mmcif_probe().
> > Is this related to the call to pm_runtime_enable() ?
> 
> The sh_mmcif_sync_reset() call accesses the hardware so it only makes 
> sense after the hardware has been resumed. Moving the clk_enable() call 
> down improves consistency by grouping clock and runtime PM code together 
> during probe() similar to other paths.

Thanks.

Reviewed-by: Simon Horman <horms@verge.net.au>

[PATCH 3/5 v4] mmc: sh_mmcif: re-read the clock frequency every time the clock is turned on

[Guennadi Liakhovetski]

With aggressive clock gating the clock can be disabled during interface
inactivity. During this time its frequency can be changed by another its
user. Therefore when the interface is activated again and the clock is
re-enabled, its frequency has to be re-read.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 6a93b04..3ffb92f 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -910,6 +910,19 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	sh_mmcif_start_cmd(host, mrq);
 }
 
+static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
+{
+	int ret = clk_enable(host->hclk);
+
+	if (!ret) {
+		host->clk = clk_get_rate(host->hclk);
+		host->mmc->f_max = host->clk / 2;
+		host->mmc->f_min = host->clk / 512;
+	}
+
+	return ret;
+}
+
 static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sh_mmcif_host *host = mmc_priv(mmc);
@@ -955,7 +968,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (!host->power) {
 			if (p->set_pwr)
 				p->set_pwr(host->pd, ios->power_mode);
-			clk_enable(host->hclk);
+			sh_mmcif_clk_update(host);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
 			sh_mmcif_sync_reset(host);
@@ -1308,10 +1321,9 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
 		goto eclkget;
 	}
-	clk_enable(host->hclk);
-	host->clk = clk_get_rate(host->hclk);
-	mmc->f_max = host->clk / 2;
-	mmc->f_min = host->clk / 512;
+	ret = sh_mmcif_clk_update(host);
+	if (ret < 0)
+		goto eclkupdate;
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
@@ -1353,6 +1365,7 @@ ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
 eresume:
 	clk_disable(host->hclk);
+eclkupdate:
 	clk_put(host->hclk);
 eclkget:
 	pm_runtime_disable(&pdev->dev);
-- 
1.7.2.5

[PATCH 3/5 v4] mmc: sh_mmcif: re-read the clock frequency every time the clock is turned on

[Guennadi Liakhovetski]

With aggressive clock gating the clock can be disabled during interface
inactivity. During this time its frequency can be changed by another its
user. Therefore when the interface is activated again and the clock is
re-enabled, its frequency has to be re-read.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 6a93b04..3ffb92f 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -910,6 +910,19 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	sh_mmcif_start_cmd(host, mrq);
 }
 
+static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
+{
+	int ret = clk_enable(host->hclk);
+
+	if (!ret) {
+		host->clk = clk_get_rate(host->hclk);
+		host->mmc->f_max = host->clk / 2;
+		host->mmc->f_min = host->clk / 512;
+	}
+
+	return ret;
+}
+
 static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sh_mmcif_host *host = mmc_priv(mmc);
@@ -955,7 +968,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (!host->power) {
 			if (p->set_pwr)
 				p->set_pwr(host->pd, ios->power_mode);
-			clk_enable(host->hclk);
+			sh_mmcif_clk_update(host);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
 			sh_mmcif_sync_reset(host);
@@ -1308,10 +1321,9 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "cannot get clock \"%s\": %d\n", clk_name, ret);
 		goto eclkget;
 	}
-	clk_enable(host->hclk);
-	host->clk = clk_get_rate(host->hclk);
-	mmc->f_max = host->clk / 2;
-	mmc->f_min = host->clk / 512;
+	ret = sh_mmcif_clk_update(host);
+	if (ret < 0)
+		goto eclkupdate;
 
 	ret = pm_runtime_resume(&pdev->dev);
 	if (ret < 0)
@@ -1353,6 +1365,7 @@ ereqirq0:
 	pm_runtime_suspend(&pdev->dev);
 eresume:
 	clk_disable(host->hclk);
+eclkupdate:
 	clk_put(host->hclk);
 eclkget:
 	pm_runtime_disable(&pdev->dev);
-- 
1.7.2.5

Re: [PATCH 3/5 v4] mmc: sh_mmcif: re-read the clock frequency every time the clock is turned on

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:23PM +0200, Guennadi Liakhovetski wrote:
> With aggressive clock gating the clock can be disabled during interface
> inactivity. During this time its frequency can be changed by another its
> user. Therefore when the interface is activated again and the clock is
> re-enabled, its frequency has to be re-read.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

Re: [PATCH 3/5 v4] mmc: sh_mmcif: re-read the clock frequency every time the clock is turned on

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:23PM +0200, Guennadi Liakhovetski wrote:
> With aggressive clock gating the clock can be disabled during interface
> inactivity. During this time its frequency can be changed by another its
> user. Therefore when the interface is activated again and the clock is
> re-enabled, its frequency has to be re-read.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

[PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Guennadi Liakhovetski]

>From the original version of sh_mmcif the .set_pwr() callback has only been
used to turn the card's power on, and the .down_pwr() callback has been
used to turn it off. .set_pwr() can be used for both these tasks, which is
also how it is implemented by the only user of this API: the SH7724 ecovec
board.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 3ffb92f..4680276 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -957,8 +957,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			pm_runtime_put(&host->pd->dev);
 			clk_disable(host->hclk);
 			host->power = false;
-			if (p->down_pwr && ios->power_mode == MMC_POWER_OFF)
-				p->down_pwr(host->pd);
+			if (p->set_pwr && ios->power_mode == MMC_POWER_OFF)
+				p->set_pwr(host->pd, 0);
 		}
 		host->state = STATE_IDLE;
 		return;
-- 
1.7.2.5

[PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Guennadi Liakhovetski]

From the original version of sh_mmcif the .set_pwr() callback has only been
used to turn the card's power on, and the .down_pwr() callback has been
used to turn it off. .set_pwr() can be used for both these tasks, which is
also how it is implemented by the only user of this API: the SH7724 ecovec
board.

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

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 3ffb92f..4680276 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -957,8 +957,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			pm_runtime_put(&host->pd->dev);
 			clk_disable(host->hclk);
 			host->power = false;
-			if (p->down_pwr && ios->power_mode = MMC_POWER_OFF)
-				p->down_pwr(host->pd);
+			if (p->set_pwr && ios->power_mode = MMC_POWER_OFF)
+				p->set_pwr(host->pd, 0);
 		}
 		host->state = STATE_IDLE;
 		return;
-- 
1.7.2.5

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> >From the original version of sh_mmcif the .set_pwr() callback has only been
> used to turn the card's power on, and the .down_pwr() callback has been
> used to turn it off. .set_pwr() can be used for both these tasks, which is
> also how it is implemented by the only user of this API: the SH7724 ecovec
> board.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

Do you also intend to post patches for the following?

* Remove mmcif_down_pwr from ecovec
* Remove down_pwr from struct sh_mmcif_plat_data

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Simon Horman]

On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> >From the original version of sh_mmcif the .set_pwr() callback has only been
> used to turn the card's power on, and the .down_pwr() callback has been
> used to turn it off. .set_pwr() can be used for both these tasks, which is
> also how it is implemented by the only user of this API: the SH7724 ecovec
> board.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Reviewed-by: Simon Horman <horms@verge.net.au>

Do you also intend to post patches for the following?

* Remove mmcif_down_pwr from ecovec
* Remove down_pwr from struct sh_mmcif_plat_data

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Guennadi Liakhovetski]

Hi Simon

On Wed, 20 Jun 2012, Simon Horman wrote:

> On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> > >From the original version of sh_mmcif the .set_pwr() callback has only been
> > used to turn the card's power on, and the .down_pwr() callback has been
> > used to turn it off. .set_pwr() can be used for both these tasks, which is
> > also how it is implemented by the only user of this API: the SH7724 ecovec
> > board.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Reviewed-by: Simon Horman <horms@verge.net.au>
> 
> Do you also intend to post patches for the following?
> 
> * Remove mmcif_down_pwr from ecovec
> * Remove down_pwr from struct sh_mmcif_plat_data

Sure, since these patches depend on each other we have to make sure this 
one gets applied first. The other two then can be applied in their time, 
they do not affect the functionality any more.

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

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Guennadi Liakhovetski]

Hi Simon

On Wed, 20 Jun 2012, Simon Horman wrote:

> On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> > >From the original version of sh_mmcif the .set_pwr() callback has only been
> > used to turn the card's power on, and the .down_pwr() callback has been
> > used to turn it off. .set_pwr() can be used for both these tasks, which is
> > also how it is implemented by the only user of this API: the SH7724 ecovec
> > board.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Reviewed-by: Simon Horman <horms@verge.net.au>
> 
> Do you also intend to post patches for the following?
> 
> * Remove mmcif_down_pwr from ecovec
> * Remove down_pwr from struct sh_mmcif_plat_data

Sure, since these patches depend on each other we have to make sure this 
one gets applied first. The other two then can be applied in their time, 
they do not affect the functionality any more.

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

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Simon Horman]

On Wed, Jun 20, 2012 at 08:19:26AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> > > >From the original version of sh_mmcif the .set_pwr() callback has only been
> > > used to turn the card's power on, and the .down_pwr() callback has been
> > > used to turn it off. .set_pwr() can be used for both these tasks, which is
> > > also how it is implemented by the only user of this API: the SH7724 ecovec
> > > board.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > Reviewed-by: Simon Horman <horms@verge.net.au>
> > 
> > Do you also intend to post patches for the following?
> > 
> > * Remove mmcif_down_pwr from ecovec
> > * Remove down_pwr from struct sh_mmcif_plat_data
> 
> Sure, since these patches depend on each other we have to make sure this 
> one gets applied first. The other two then can be applied in their time, 
> they do not affect the functionality any more.

Thanks, I agree entirely.

Re: [PATCH 4/5 v4] mmc: sh_mmcif: remove redundant .down_pwr() callback

[Simon Horman]

On Wed, Jun 20, 2012 at 08:19:26AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > On Wed, Jun 13, 2012 at 03:37:27PM +0200, Guennadi Liakhovetski wrote:
> > > >From the original version of sh_mmcif the .set_pwr() callback has only been
> > > used to turn the card's power on, and the .down_pwr() callback has been
> > > used to turn it off. .set_pwr() can be used for both these tasks, which is
> > > also how it is implemented by the only user of this API: the SH7724 ecovec
> > > board.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > Reviewed-by: Simon Horman <horms@verge.net.au>
> > 
> > Do you also intend to post patches for the following?
> > 
> > * Remove mmcif_down_pwr from ecovec
> > * Remove down_pwr from struct sh_mmcif_plat_data
> 
> Sure, since these patches depend on each other we have to make sure this 
> one gets applied first. The other two then can be applied in their time, 
> they do not affect the functionality any more.

Thanks, I agree entirely.

[PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Guennadi Liakhovetski]

Add regulator support to the sh_mmcif driver, but also preserve the current
power-callback.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v4: update to the new mmc_regulator_get_supply() function

 drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 4680276..204bced 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
 	return ret;
 }
 
+static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
+{
+	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
+	struct mmc_host *mmc = host->mmc;
+
+	if (pd->set_pwr)
+		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
+	if (!IS_ERR(mmc->supply.vmmc))
+		/* Errors ignored... */
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
+				      ios->power_mode ? ios->vdd : 0);
+}
+
 static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sh_mmcif_host *host = mmc_priv(mmc);
-	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
@@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
 			host->card_present = true;
 		}
+		sh_mmcif_set_power(host, ios);
 	} else if (ios->power_mode == MMC_POWER_OFF || !ios->clock) {
 		/* clock stop */
 		sh_mmcif_clock_control(host, 0);
@@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			pm_runtime_put(&host->pd->dev);
 			clk_disable(host->hclk);
 			host->power = false;
-			if (p->set_pwr && ios->power_mode == MMC_POWER_OFF)
-				p->set_pwr(host->pd, 0);
+			if (ios->power_mode == MMC_POWER_OFF)
+				sh_mmcif_set_power(host, ios);
 		}
 		host->state = STATE_IDLE;
 		return;
@@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (ios->clock) {
 		if (!host->power) {
-			if (p->set_pwr)
-				p->set_pwr(host->pd, ios->power_mode);
 			sh_mmcif_clk_update(host);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
@@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
 	mmc_request_done(host->mmc, mrq);
 }
 
+static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
+{
+	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
+	struct mmc_host *mmc = host->mmc;
+
+	mmc_regulator_get_supply(mmc);
+
+	if (!mmc->ocr_avail)
+		mmc->ocr_avail = pd->ocr;
+	else if (pd->ocr)
+		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
+}
+
 static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 {
 	int ret = 0, irq[2];
@@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	spin_lock_init(&host->lock);
 
 	mmc->ops = &sh_mmcif_ops;
-	if (pd->ocr)
-		mmc->ocr_avail = pd->ocr;
+	sh_mmcif_init_ocr(host);
+
 	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
 	if (pd->caps)
 		mmc->caps |= pd->caps;
-- 
1.7.2.5

[PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Guennadi Liakhovetski]

Add regulator support to the sh_mmcif driver, but also preserve the current
power-callback.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v4: update to the new mmc_regulator_get_supply() function

 drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 4680276..204bced 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
 	return ret;
 }
 
+static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
+{
+	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
+	struct mmc_host *mmc = host->mmc;
+
+	if (pd->set_pwr)
+		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
+	if (!IS_ERR(mmc->supply.vmmc))
+		/* Errors ignored... */
+		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
+				      ios->power_mode ? ios->vdd : 0);
+}
+
 static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sh_mmcif_host *host = mmc_priv(mmc);
-	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
@@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
 			host->card_present = true;
 		}
+		sh_mmcif_set_power(host, ios);
 	} else if (ios->power_mode = MMC_POWER_OFF || !ios->clock) {
 		/* clock stop */
 		sh_mmcif_clock_control(host, 0);
@@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			pm_runtime_put(&host->pd->dev);
 			clk_disable(host->hclk);
 			host->power = false;
-			if (p->set_pwr && ios->power_mode = MMC_POWER_OFF)
-				p->set_pwr(host->pd, 0);
+			if (ios->power_mode = MMC_POWER_OFF)
+				sh_mmcif_set_power(host, ios);
 		}
 		host->state = STATE_IDLE;
 		return;
@@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	if (ios->clock) {
 		if (!host->power) {
-			if (p->set_pwr)
-				p->set_pwr(host->pd, ios->power_mode);
 			sh_mmcif_clk_update(host);
 			pm_runtime_get_sync(&host->pd->dev);
 			host->power = true;
@@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
 	mmc_request_done(host->mmc, mrq);
 }
 
+static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
+{
+	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
+	struct mmc_host *mmc = host->mmc;
+
+	mmc_regulator_get_supply(mmc);
+
+	if (!mmc->ocr_avail)
+		mmc->ocr_avail = pd->ocr;
+	else if (pd->ocr)
+		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
+}
+
 static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 {
 	int ret = 0, irq[2];
@@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 	spin_lock_init(&host->lock);
 
 	mmc->ops = &sh_mmcif_ops;
-	if (pd->ocr)
-		mmc->ocr_avail = pd->ocr;
+	sh_mmcif_init_ocr(host);
+
 	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
 	if (pd->caps)
 		mmc->caps |= pd->caps;
-- 
1.7.2.5

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> Add regulator support to the sh_mmcif driver, but also preserve the current
> power-callback.

Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> Add regulator support to the sh_mmcif driver, but also preserve the current
> power-callback.

Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Simon Horman]

Hi Guennadi,

On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> Add regulator support to the sh_mmcif driver, but also preserve the current
> power-callback.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> v4: update to the new mmc_regulator_get_supply() function
> 
>  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
>  1 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 4680276..204bced 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
>  	return ret;
>  }
>  
> +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> +{
> +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	if (pd->set_pwr)
> +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> +	if (!IS_ERR(mmc->supply.vmmc))
> +		/* Errors ignored... */
> +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +				      ios->power_mode ? ios->vdd : 0);
> +}
> +
>  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>  	struct sh_mmcif_host *host = mmc_priv(mmc);
> -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
>  			host->card_present = true;
>  		}
> +		sh_mmcif_set_power(host, ios);
>  	} else if (ios->power_mode == MMC_POWER_OFF || !ios->clock) {
>  		/* clock stop */
>  		sh_mmcif_clock_control(host, 0);
> @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  			pm_runtime_put(&host->pd->dev);
>  			clk_disable(host->hclk);
>  			host->power = false;
> -			if (p->set_pwr && ios->power_mode == MMC_POWER_OFF)
> -				p->set_pwr(host->pd, 0);
> +			if (ios->power_mode == MMC_POWER_OFF)
> +				sh_mmcif_set_power(host, ios);
>  		}
>  		host->state = STATE_IDLE;
>  		return;
> @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  
>  	if (ios->clock) {
>  		if (!host->power) {
> -			if (p->set_pwr)
> -				p->set_pwr(host->pd, ios->power_mode);
>  			sh_mmcif_clk_update(host);
>  			pm_runtime_get_sync(&host->pd->dev);
>  			host->power = true;

It is unclear to me how this hunk relates to the description of the
change in the changelog.

> @@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
>  	mmc_request_done(host->mmc, mrq);
>  }
>  
> +static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
> +{
> +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	mmc_regulator_get_supply(mmc);
> +
> +	if (!mmc->ocr_avail)
> +		mmc->ocr_avail = pd->ocr;
> +	else if (pd->ocr)
> +		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
> +}
> +
>  static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  {
>  	int ret = 0, irq[2];
> @@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	spin_lock_init(&host->lock);
>  
>  	mmc->ops = &sh_mmcif_ops;
> -	if (pd->ocr)
> -		mmc->ocr_avail = pd->ocr;
> +	sh_mmcif_init_ocr(host);
> +
>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
>  	if (pd->caps)
>  		mmc->caps |= pd->caps;
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Simon Horman]

Hi Guennadi,

On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> Add regulator support to the sh_mmcif driver, but also preserve the current
> power-callback.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> v4: update to the new mmc_regulator_get_supply() function
> 
>  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
>  1 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 4680276..204bced 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
>  	return ret;
>  }
>  
> +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> +{
> +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	if (pd->set_pwr)
> +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> +	if (!IS_ERR(mmc->supply.vmmc))
> +		/* Errors ignored... */
> +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +				      ios->power_mode ? ios->vdd : 0);
> +}
> +
>  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>  	struct sh_mmcif_host *host = mmc_priv(mmc);
> -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
>  			host->card_present = true;
>  		}
> +		sh_mmcif_set_power(host, ios);
>  	} else if (ios->power_mode = MMC_POWER_OFF || !ios->clock) {
>  		/* clock stop */
>  		sh_mmcif_clock_control(host, 0);
> @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  			pm_runtime_put(&host->pd->dev);
>  			clk_disable(host->hclk);
>  			host->power = false;
> -			if (p->set_pwr && ios->power_mode = MMC_POWER_OFF)
> -				p->set_pwr(host->pd, 0);
> +			if (ios->power_mode = MMC_POWER_OFF)
> +				sh_mmcif_set_power(host, ios);
>  		}
>  		host->state = STATE_IDLE;
>  		return;
> @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  
>  	if (ios->clock) {
>  		if (!host->power) {
> -			if (p->set_pwr)
> -				p->set_pwr(host->pd, ios->power_mode);
>  			sh_mmcif_clk_update(host);
>  			pm_runtime_get_sync(&host->pd->dev);
>  			host->power = true;

It is unclear to me how this hunk relates to the description of the
change in the changelog.

> @@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
>  	mmc_request_done(host->mmc, mrq);
>  }
>  
> +static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
> +{
> +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	mmc_regulator_get_supply(mmc);
> +
> +	if (!mmc->ocr_avail)
> +		mmc->ocr_avail = pd->ocr;
> +	else if (pd->ocr)
> +		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
> +}
> +
>  static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  {
>  	int ret = 0, irq[2];
> @@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  	spin_lock_init(&host->lock);
>  
>  	mmc->ops = &sh_mmcif_ops;
> -	if (pd->ocr)
> -		mmc->ocr_avail = pd->ocr;
> +	sh_mmcif_init_ocr(host);
> +
>  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
>  	if (pd->caps)
>  		mmc->caps |= pd->caps;
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Guennadi Liakhovetski]

On Wed, 20 Jun 2012, Simon Horman wrote:

> Hi Guennadi,
> 
> On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> > Add regulator support to the sh_mmcif driver, but also preserve the current
> > power-callback.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > v4: update to the new mmc_regulator_get_supply() function
> > 
> >  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
> >  1 files changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > index 4680276..204bced 100644
> > --- a/drivers/mmc/host/sh_mmcif.c
> > +++ b/drivers/mmc/host/sh_mmcif.c
> > @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
> >  	return ret;
> >  }
> >  
> > +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> > +{
> > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > +	struct mmc_host *mmc = host->mmc;
> > +
> > +	if (pd->set_pwr)
> > +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> > +	if (!IS_ERR(mmc->supply.vmmc))
> > +		/* Errors ignored... */
> > +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> > +				      ios->power_mode ? ios->vdd : 0);
> > +}
> > +
> >  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  {
> >  	struct sh_mmcif_host *host = mmc_priv(mmc);
> > -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&host->lock, flags);
> > @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
> >  			host->card_present = true;
> >  		}
> > +		sh_mmcif_set_power(host, ios);
> >  	} else if (ios->power_mode == MMC_POWER_OFF || !ios->clock) {
> >  		/* clock stop */
> >  		sh_mmcif_clock_control(host, 0);
> > @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  			pm_runtime_put(&host->pd->dev);
> >  			clk_disable(host->hclk);
> >  			host->power = false;
> > -			if (p->set_pwr && ios->power_mode == MMC_POWER_OFF)
> > -				p->set_pwr(host->pd, 0);
> > +			if (ios->power_mode == MMC_POWER_OFF)
> > +				sh_mmcif_set_power(host, ios);
> >  		}
> >  		host->state = STATE_IDLE;
> >  		return;
> > @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  
> >  	if (ios->clock) {
> >  		if (!host->power) {
> > -			if (p->set_pwr)
> > -				p->set_pwr(host->pd, ios->power_mode);
> >  			sh_mmcif_clk_update(host);
> >  			pm_runtime_get_sync(&host->pd->dev);
> >  			host->power = true;
> 
> It is unclear to me how this hunk relates to the description of the
> change in the changelog.

Right, it is probably not very obvious. We could extend the description 
with something like:

Also note, that the card power is not switched off during clock gating 
periods, hence there's no need to power it on every time the card is 
re-activated.

Thanks
Guennadi

> 
> > @@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
> >  	mmc_request_done(host->mmc, mrq);
> >  }
> >  
> > +static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
> > +{
> > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > +	struct mmc_host *mmc = host->mmc;
> > +
> > +	mmc_regulator_get_supply(mmc);
> > +
> > +	if (!mmc->ocr_avail)
> > +		mmc->ocr_avail = pd->ocr;
> > +	else if (pd->ocr)
> > +		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
> > +}
> > +
> >  static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  {
> >  	int ret = 0, irq[2];
> > @@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	spin_lock_init(&host->lock);
> >  
> >  	mmc->ops = &sh_mmcif_ops;
> > -	if (pd->ocr)
> > -		mmc->ocr_avail = pd->ocr;
> > +	sh_mmcif_init_ocr(host);
> > +
> >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> >  	if (pd->caps)
> >  		mmc->caps |= pd->caps;
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

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

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Guennadi Liakhovetski]

On Wed, 20 Jun 2012, Simon Horman wrote:

> Hi Guennadi,
> 
> On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> > Add regulator support to the sh_mmcif driver, but also preserve the current
> > power-callback.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > v4: update to the new mmc_regulator_get_supply() function
> > 
> >  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
> >  1 files changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > index 4680276..204bced 100644
> > --- a/drivers/mmc/host/sh_mmcif.c
> > +++ b/drivers/mmc/host/sh_mmcif.c
> > @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
> >  	return ret;
> >  }
> >  
> > +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> > +{
> > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > +	struct mmc_host *mmc = host->mmc;
> > +
> > +	if (pd->set_pwr)
> > +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> > +	if (!IS_ERR(mmc->supply.vmmc))
> > +		/* Errors ignored... */
> > +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> > +				      ios->power_mode ? ios->vdd : 0);
> > +}
> > +
> >  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  {
> >  	struct sh_mmcif_host *host = mmc_priv(mmc);
> > -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&host->lock, flags);
> > @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
> >  			host->card_present = true;
> >  		}
> > +		sh_mmcif_set_power(host, ios);
> >  	} else if (ios->power_mode = MMC_POWER_OFF || !ios->clock) {
> >  		/* clock stop */
> >  		sh_mmcif_clock_control(host, 0);
> > @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  			pm_runtime_put(&host->pd->dev);
> >  			clk_disable(host->hclk);
> >  			host->power = false;
> > -			if (p->set_pwr && ios->power_mode = MMC_POWER_OFF)
> > -				p->set_pwr(host->pd, 0);
> > +			if (ios->power_mode = MMC_POWER_OFF)
> > +				sh_mmcif_set_power(host, ios);
> >  		}
> >  		host->state = STATE_IDLE;
> >  		return;
> > @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >  
> >  	if (ios->clock) {
> >  		if (!host->power) {
> > -			if (p->set_pwr)
> > -				p->set_pwr(host->pd, ios->power_mode);
> >  			sh_mmcif_clk_update(host);
> >  			pm_runtime_get_sync(&host->pd->dev);
> >  			host->power = true;
> 
> It is unclear to me how this hunk relates to the description of the
> change in the changelog.

Right, it is probably not very obvious. We could extend the description 
with something like:

Also note, that the card power is not switched off during clock gating 
periods, hence there's no need to power it on every time the card is 
re-activated.

Thanks
Guennadi

> 
> > @@ -1251,6 +1262,19 @@ static void mmcif_timeout_work(struct work_struct *work)
> >  	mmc_request_done(host->mmc, mrq);
> >  }
> >  
> > +static void sh_mmcif_init_ocr(struct sh_mmcif_host *host)
> > +{
> > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > +	struct mmc_host *mmc = host->mmc;
> > +
> > +	mmc_regulator_get_supply(mmc);
> > +
> > +	if (!mmc->ocr_avail)
> > +		mmc->ocr_avail = pd->ocr;
> > +	else if (pd->ocr)
> > +		dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
> > +}
> > +
> >  static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  {
> >  	int ret = 0, irq[2];
> > @@ -1298,8 +1322,8 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
> >  	spin_lock_init(&host->lock);
> >  
> >  	mmc->ops = &sh_mmcif_ops;
> > -	if (pd->ocr)
> > -		mmc->ocr_avail = pd->ocr;
> > +	sh_mmcif_init_ocr(host);
> > +
> >  	mmc->caps = MMC_CAP_MMC_HIGHSPEED;
> >  	if (pd->caps)
> >  		mmc->caps |= pd->caps;
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

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

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Simon Horman]

On Wed, Jun 20, 2012 at 08:38:27AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > Hi Guennadi,
> > 
> > On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> > > Add regulator support to the sh_mmcif driver, but also preserve the current
> > > power-callback.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > > 
> > > v4: update to the new mmc_regulator_get_supply() function
> > > 
> > >  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
> > >  1 files changed, 31 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > > index 4680276..204bced 100644
> > > --- a/drivers/mmc/host/sh_mmcif.c
> > > +++ b/drivers/mmc/host/sh_mmcif.c
> > > @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
> > >  	return ret;
> > >  }
> > >  
> > > +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> > > +{
> > > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > > +	struct mmc_host *mmc = host->mmc;
> > > +
> > > +	if (pd->set_pwr)
> > > +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> > > +	if (!IS_ERR(mmc->supply.vmmc))
> > > +		/* Errors ignored... */
> > > +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> > > +				      ios->power_mode ? ios->vdd : 0);
> > > +}
> > > +
> > >  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  {
> > >  	struct sh_mmcif_host *host = mmc_priv(mmc);
> > > -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&host->lock, flags);
> > > @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
> > >  			host->card_present = true;
> > >  		}
> > > +		sh_mmcif_set_power(host, ios);
> > >  	} else if (ios->power_mode == MMC_POWER_OFF || !ios->clock) {
> > >  		/* clock stop */
> > >  		sh_mmcif_clock_control(host, 0);
> > > @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  			pm_runtime_put(&host->pd->dev);
> > >  			clk_disable(host->hclk);
> > >  			host->power = false;
> > > -			if (p->set_pwr && ios->power_mode == MMC_POWER_OFF)
> > > -				p->set_pwr(host->pd, 0);
> > > +			if (ios->power_mode == MMC_POWER_OFF)
> > > +				sh_mmcif_set_power(host, ios);
> > >  		}
> > >  		host->state = STATE_IDLE;
> > >  		return;
> > > @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  
> > >  	if (ios->clock) {
> > >  		if (!host->power) {
> > > -			if (p->set_pwr)
> > > -				p->set_pwr(host->pd, ios->power_mode);
> > >  			sh_mmcif_clk_update(host);
> > >  			pm_runtime_get_sync(&host->pd->dev);
> > >  			host->power = true;
> > 
> > It is unclear to me how this hunk relates to the description of the
> > change in the changelog.
> 
> Right, it is probably not very obvious. We could extend the description 
> with something like:
> 
> Also note, that the card power is not switched off during clock gating 
> periods, hence there's no need to power it on every time the card is 
> re-activated.

Thanks, that would make things much clearer.

Reviewed-by: Simon Horman <horms@verge.net.au>

Re: [PATCH 5/5 v4] mmc: sh_mmcif: add regulator support

[Simon Horman]

On Wed, Jun 20, 2012 at 08:38:27AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > Hi Guennadi,
> > 
> > On Wed, Jun 13, 2012 at 03:37:31PM +0200, Guennadi Liakhovetski wrote:
> > > Add regulator support to the sh_mmcif driver, but also preserve the current
> > > power-callback.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > > 
> > > v4: update to the new mmc_regulator_get_supply() function
> > > 
> > >  drivers/mmc/host/sh_mmcif.c |   38 +++++++++++++++++++++++++++++++-------
> > >  1 files changed, 31 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> > > index 4680276..204bced 100644
> > > --- a/drivers/mmc/host/sh_mmcif.c
> > > +++ b/drivers/mmc/host/sh_mmcif.c
> > > @@ -923,10 +923,22 @@ static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
> > >  	return ret;
> > >  }
> > >  
> > > +static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios *ios)
> > > +{
> > > +	struct sh_mmcif_plat_data *pd = host->pd->dev.platform_data;
> > > +	struct mmc_host *mmc = host->mmc;
> > > +
> > > +	if (pd->set_pwr)
> > > +		pd->set_pwr(host->pd, ios->power_mode != MMC_POWER_OFF);
> > > +	if (!IS_ERR(mmc->supply.vmmc))
> > > +		/* Errors ignored... */
> > > +		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> > > +				      ios->power_mode ? ios->vdd : 0);
> > > +}
> > > +
> > >  static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  {
> > >  	struct sh_mmcif_host *host = mmc_priv(mmc);
> > > -	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&host->lock, flags);
> > > @@ -944,6 +956,7 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  			sh_mmcif_request_dma(host, host->pd->dev.platform_data);
> > >  			host->card_present = true;
> > >  		}
> > > +		sh_mmcif_set_power(host, ios);
> > >  	} else if (ios->power_mode = MMC_POWER_OFF || !ios->clock) {
> > >  		/* clock stop */
> > >  		sh_mmcif_clock_control(host, 0);
> > > @@ -957,8 +970,8 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  			pm_runtime_put(&host->pd->dev);
> > >  			clk_disable(host->hclk);
> > >  			host->power = false;
> > > -			if (p->set_pwr && ios->power_mode = MMC_POWER_OFF)
> > > -				p->set_pwr(host->pd, 0);
> > > +			if (ios->power_mode = MMC_POWER_OFF)
> > > +				sh_mmcif_set_power(host, ios);
> > >  		}
> > >  		host->state = STATE_IDLE;
> > >  		return;
> > > @@ -966,8 +979,6 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > >  
> > >  	if (ios->clock) {
> > >  		if (!host->power) {
> > > -			if (p->set_pwr)
> > > -				p->set_pwr(host->pd, ios->power_mode);
> > >  			sh_mmcif_clk_update(host);
> > >  			pm_runtime_get_sync(&host->pd->dev);
> > >  			host->power = true;
> > 
> > It is unclear to me how this hunk relates to the description of the
> > change in the changelog.
> 
> Right, it is probably not very obvious. We could extend the description 
> with something like:
> 
> Also note, that the card power is not switched off during clock gating 
> periods, hence there's no need to power it on every time the card is 
> re-activated.

Thanks, that would make things much clearer.

Reviewed-by: Simon Horman <horms@verge.net.au>