diff mbox

[1/3] mmc: sh_mmcif: Make sure the device stays active when needed in ->probe()

Message ID 1455195595-5121-1-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Feb. 11, 2016, 12:59 p.m. UTC 
While accessing the device, make sure it stays active by increasing the
runtime PM usage count for it.

Let's also defer to enable runtime PM until we really need access to the
device. This also enables the error path in ->probe() to become simpler.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/mmc/host/sh_mmcif.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
1.9.1

Comments

Ulf Hansson March 3, 2016, 2:59 p.m. UTC | #1 
On 11 February 2016 at 13:59, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> While accessing the device, make sure it stays active by increasing the

> runtime PM usage count for it.

>

> Let's also defer to enable runtime PM until we really need access to the

> device. This also enables the error path in ->probe() to become simpler.

>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Nguyen, sorry for bothering you. Did you manage to test out these three patches?

Even if it doesn't solve the original issue that you reported [1],
perhaps these can anyway be useful as cleanups?

Kind regards
Uffe

[1]
http://www.spinics.net/lists/linux-sh/msg47766.html

> ---

>  drivers/mmc/host/sh_mmcif.c | 15 ++++++++-------

>  1 file changed, 8 insertions(+), 7 deletions(-)

>

> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c

> index 8d870ce..c6c2a08 100644

> --- a/drivers/mmc/host/sh_mmcif.c

> +++ b/drivers/mmc/host/sh_mmcif.c

> @@ -1519,23 +1519,23 @@ static int sh_mmcif_probe(struct platform_device *pdev)

>

>         platform_set_drvdata(pdev, host);

>

> -       pm_runtime_enable(dev);

> -       host->power = false;

> -

>         host->clk = devm_clk_get(dev, NULL);

>         if (IS_ERR(host->clk)) {

>                 ret = PTR_ERR(host->clk);

>                 dev_err(dev, "cannot get clock: %d\n", ret);

> -               goto err_pm;

> +               goto err_host;

>         }

>

>         ret = clk_prepare_enable(host->clk);

>         if (ret < 0)

> -               goto err_pm;

> +               goto err_host;

>

>         sh_mmcif_clk_setup(host);

>

> -       ret = pm_runtime_resume(dev);

> +       pm_runtime_enable(dev);

> +       host->power = false;

> +

> +       ret = pm_runtime_get_sync(dev);

>         if (ret < 0)

>                 goto err_clk;

>

> @@ -1579,12 +1579,13 @@ static int sh_mmcif_probe(struct platform_device *pdev)

>                  sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0xffff,

>                  clk_get_rate(host->clk) / 1000000UL);

>

> +       pm_runtime_put(dev);

>         clk_disable_unprepare(host->clk);

>         return ret;

>

>  err_clk:

>         clk_disable_unprepare(host->clk);

> -err_pm:

> +       pm_runtime_put_sync(dev);

>         pm_runtime_disable(dev);

>  err_host:

>         mmc_free_host(mmc);

> --

> 1.9.1

>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson March 16, 2016, 9:53 a.m. UTC | #2 
On 7 March 2016 at 04:17, Nguyen Viet Dung <nv-dung@jinso.co.jp> wrote:
> Hi Ulf

>

> I have testted two cases use these patchs that you send.

>

> case01:

> [PATCH 1/3] mmc: sh_mmcif: Make sure the device stays active when needed in

> ->probe()

> [PATCH 2/3] mmc: sh_mmcif: Restructure ->set_ios()

> [PATCH 3/3] mmc: sh_mmci: Get rid of wrapper function for regulators

>

> case02:

> [PATCH 1/3] mmc: sh_mmcif: Make sure the device stays active when needed in

> ->probe()

>

> The problem of mmcif is not resolved by both two cases.


Okay, thanks for testing!

Even if it doesn't solve you original issue after a system PM resumed
has been accomplished, does the driver still works as before? In other
words are cards detected properly and can data transfers be done?

The reason for my question, is that I think these patches makes some
nice cleanups of some of the PM related code in the driver. So unless
you see a *new* regression, I would like to apply them.

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson April 5, 2016, 11:11 a.m. UTC | #3 
On 17 March 2016 at 08:36, Nguyen Viet Dung <nv-dung@jinso.co.jp> wrote:
> Hi Ulf,

>

> Thank for your reply.

>

> On 2016年03月16日 18:53, Ulf Hansson wrote:

>>

>> On 7 March 2016 at 04:17, Nguyen Viet Dung <nv-dung@jinso.co.jp> wrote:

>>>

>>> Hi Ulf

>>>

>>> I have testted two cases use these patchs that you send.

>>>

>>> case01:

>>> [PATCH 1/3] mmc: sh_mmcif: Make sure the device stays active when needed

>>> in

>>> ->probe()

>>> [PATCH 2/3] mmc: sh_mmcif: Restructure ->set_ios()

>>> [PATCH 3/3] mmc: sh_mmci: Get rid of wrapper function for regulators

>>>

>>> case02:

>>> [PATCH 1/3] mmc: sh_mmcif: Make sure the device stays active when needed

>>> in

>>> ->probe()

>>>

>>> The problem of mmcif is not resolved by both two cases.

>>

>> Okay, thanks for testing!

>>

>> Even if it doesn't solve you original issue after a system PM resumed

>> has been accomplished, does the driver still works as before? In other

>> words are cards detected properly and can data transfers be done?

>

> After a system PM resumed has been accomplished,if write data to mmc then

> happen kernel panic.

>>

>> The reason for my question, is that I think these patches makes some

>> nice cleanups of some of the PM related code in the driver. So unless

>> you see a *new* regression, I would like to apply them.

>

> If see only this problem of mmc,i can not know effect of these patchs.


As I understand your test, there are no new issues observed. For that
reason, I am going to apply patch 1->3 for next to give them some more
testing. Please tell me if any new issues pops up.

Regarding the issue you reported as a regression, unfortunate it seems
like someone with the hardware at hand needs to some debugging.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 8d870ce..c6c2a08 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1519,23 +1519,23 @@  static int sh_mmcif_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, host);
 
-	pm_runtime_enable(dev);
-	host->power = false;
-
 	host->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(host->clk)) {
 		ret = PTR_ERR(host->clk);
 		dev_err(dev, "cannot get clock: %d\n", ret);
-		goto err_pm;
+		goto err_host;
 	}
 
 	ret = clk_prepare_enable(host->clk);
 	if (ret < 0)
-		goto err_pm;
+		goto err_host;
 
 	sh_mmcif_clk_setup(host);
 
-	ret = pm_runtime_resume(dev);
+	pm_runtime_enable(dev);
+	host->power = false;
+
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err_clk;
 
@@ -1579,12 +1579,13 @@  static int sh_mmcif_probe(struct platform_device *pdev)
 		 sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0xffff,
 		 clk_get_rate(host->clk) / 1000000UL);
 
+	pm_runtime_put(dev);
 	clk_disable_unprepare(host->clk);
 	return ret;
 
 err_clk:
 	clk_disable_unprepare(host->clk);
-err_pm:
+	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 err_host:
 	mmc_free_host(mmc);