Question about commit "mmc: core: Support aggressive power management for (e)MMC/SD"

PATCH Axel Lin 2 participants 2 messages Oct 6, 2013 MMC, SD and SDIO

Share & tools

Axel Lin

author

2013-10-06 04:07 UTC #0 lore raw

Hi Ulf,

While reading the code, I don't figure out why we need a nested mmc_claim_host /mmc_release_host calls and additional mmc_power_off/mmc_power_up calls in commit c4d770d72.

The implementation of mmc_suspend/mmc_resume and mmc_sd_suspend/mmc_sd_resume already calls mmc_claim_host/mmc_release_host and mmc_power_off/mmc_power_up.

I don't have a hardware handy to test it at this moment. I'm wondering if it still works with below diff:

drivers/mmc/core/mmc.c +1 ~~-12~~

index 6d02012..f0dcc53 100644
@@ -1552,18 +1552,11 @@ static int mmc_runtime_suspend(struct mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
 	err = mmc_suspend(host);
-	if (err) {
+	if (err)
 		pr_err("%s: error %d doing aggessive suspend\n",
 			mmc_hostname(host), err);
-		goto out;
-	}
-	mmc_power_off(host);
 
-out:
-	mmc_release_host(host);
 	return err;
 }

@@ -1577,15 +1570,11 @@ static int mmc_runtime_resume(struct mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
-	mmc_power_up(host);
 	err = mmc_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
-	mmc_release_host(host);
 	return 0;
 }

drivers/mmc/core/sd.c +1 ~~-12~~

index 5e8823d..0f2a1f9 100644
@@ -1117,18 +1117,11 @@ static int mmc_sd_runtime_suspend(struct mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
 	err = mmc_sd_suspend(host);
-	if (err) {
+	if (err)
 		pr_err("%s: error %d doing aggessive suspend\n",
 			mmc_hostname(host), err);
-		goto out;
-	}
-	mmc_power_off(host);
 
-out:
-	mmc_release_host(host);
 	return err;
 }

@@ -1142,15 +1135,11 @@ static int mmc_sd_runtime_resume(struct mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
-	mmc_power_up(host);
 	err = mmc_sd_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
-	mmc_release_host(host);
 	return 0;
 }

Ulf Hansson

2013-10-06 12:57 UTC #1 lore raw

Axel Lin <[email protected]> skrev:

Quoted 7 lines

Hi Ulf,
While reading the code, I don't figure out why we need a nested mmc_claim_host
/mmc_release_host calls and additional mmc_power_off/mmc_power_up calls in
commit c4d770d72.

Hi Axel,

Please have a look at a patch I sent 3 October. "mmc: core: Remove redundant mmc_power_.....

You have spotted that this code is not needed, I believe the commitmsg in the patch above will give you some background, to why it looked like it did.

Kind regards
Ulf Hansson

Quoted 93 lines

The implementation of mmc_suspend/mmc_resume and
mmc_sd_suspend/mmc_sd_resume
already calls mmc_claim_host/mmc_release_host and
mmc_power_off/mmc_power_up.

I don't have a hardware handy to test it at this moment. I'm wondering if it still works with below diff:

drivers/mmc/core/mmc.c +1 ~~-12~~

index 6d02012..f0dcc53 100644
@@ -1552,18 +1552,11 @@ static int mmc_runtime_suspend(struct mmc_host
 *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
 	err = mmc_suspend(host);
-	if (err) {
+	if (err)
 		pr_err("%s: error %d doing aggessive suspend\n",
 			mmc_hostname(host), err);
-		goto out;
-	}
-	mmc_power_off(host);
 
-out:
-	mmc_release_host(host);
 	return err;
 }

@@ -1577,15 +1570,11 @@ static int mmc_runtime_resume(struct mmc_host
 *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
-	mmc_power_up(host);
 	err = mmc_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
-	mmc_release_host(host);
 	return 0;
 }

drivers/mmc/core/sd.c +1 ~~-12~~

index 5e8823d..0f2a1f9 100644
@@ -1117,18 +1117,11 @@ static int mmc_sd_runtime_suspend(struct
 mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
 	err = mmc_sd_suspend(host);
-	if (err) {
+	if (err)
 		pr_err("%s: error %d doing aggessive suspend\n",
 			mmc_hostname(host), err);
-		goto out;
-	}
-	mmc_power_off(host);
 
-out:
-	mmc_release_host(host);
 	return err;
 }

@@ -1142,15 +1135,11 @@ static int mmc_sd_runtime_resume(struct
 mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	mmc_claim_host(host);
-
-	mmc_power_up(host);
 	err = mmc_sd_resume(host);
 	if (err)
 		pr_err("%s: error %d doing aggessive resume\n",
 			mmc_hostname(host), err);
 
-	mmc_release_host(host);
 	return 0;
 }