diff mbox

[v2] mmc: dw_mmc: Disable low power mode if SDIO interrupts are used

Message ID 1343062955-24516-1-git-send-email-dianders@chromium.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Doug Anderson July 23, 2012, 5:02 p.m. UTC 
The documentation for the dw_mmc part says that the low power
mode should normally only be set for MMC and SD memory and should
be turned off for SDIO cards that need interrupts detected.

The best place I could find to do this is when the SDIO interrupt
was first enabled.  I rely on the fact that dw_mci_setup_bus()
will be called when it's time to reenable.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Commenting fixes requested by Grant Grundler.
- Be extra certain that we don't re-turn on the low power mode in
  CLKENA in dw_mci_setup_bus() if SDIO interrupts are enabled.
  There are no known instances of this happening but it's good to be safe.

 drivers/mmc/host/dw_mmc.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

Comments

Seungwon Jeon July 24, 2012, 1:17 a.m. UTC | #1 
July 24, 2012, Doug Anderson <dianders@chromium.org> wrote:
> The documentation for the dw_mmc part says that the low power
> mode should normally only be set for MMC and SD memory and should
> be turned off for SDIO cards that need interrupts detected.
> 
> The best place I could find to do this is when the SDIO interrupt
> was first enabled.  I rely on the fact that dw_mci_setup_bus()
> will be called when it's time to reenable.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Commenting fixes requested by Grant Grundler.
> - Be extra certain that we don't re-turn on the low power mode in
>   CLKENA in dw_mci_setup_bus() if SDIO interrupts are enabled.
>   There are no known instances of this happening but it's good to be safe.
> 
>  drivers/mmc/host/dw_mmc.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 72dc3cd..0ab1771 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -627,6 +627,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>  {
>  	struct dw_mci *host = slot->host;
>  	u32 div;
> +	u32 clk_en_a;
> 
>  	if (slot->clock != host->current_speed) {
>  		div = host->bus_hz / slot->clock;
> @@ -659,9 +660,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>  		mci_send_cmd(slot,
>  			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
> 
> -		/* enable clock */
> -		mci_writel(host, CLKENA, ((SDMMC_CLKEN_ENABLE |
> -			   SDMMC_CLKEN_LOW_PWR) << slot->id));
> +		/* enable clock; only low power if no SDIO */
> +		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
> +		if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id)))
> +			clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
> +		mci_writel(host, CLKENA, clk_en_a);
I have followed this patch from v1.
I think it's a good point.
Looks good to me.

> 
>  		/* inform CIU */
>  		mci_send_cmd(slot,
> @@ -862,6 +865,32 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	return present;
>  }
> 
> +/*
> + * Disable lower power mode.
> + *
> + * Low power mode will stop the card clock when idle.  According to
> + * documentation (Exynos 5250 User's Manual 0.04, description of
> + * CLKENA register) we should disable low power mode for SDIO cards
> + * if we need interrupts to work.
Above comment is correct, but this is not specific for Exynos5250.
Exynos5250 is just one of host controllers base on Synopsys's.
It'd be better to remove the part related to Exynos you mentioned .


> + *
> + * This function is fast if the power mode is already disabled.
Definitely, low power mode not power mode, right?

Best regards,
Seungwon Jeon

> + */
> +static void dw_mci_disable_low_power(struct mmc_host *mmc)
> +{
> +	struct dw_mci_slot *slot = mmc_priv(mmc);
> +	struct dw_mci *host = slot->host;
> +	u32 clk_en_a;
> +	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> +
> +	clk_en_a = mci_readl(host, CLKENA);
> +
> +	if (clk_en_a & clken_low_pwr) {
> +		mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> +		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> +			     SDMMC_CMD_PRV_DAT_WAIT, 0);
> +	}
> +}
> +
>  static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  {
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
> @@ -871,6 +900,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  	/* Enable/disable Slot Specific SDIO interrupt */
>  	int_mask = mci_readl(host, INTMASK);
>  	if (enb) {
> +		/*
> +		 * Turn off low power mode if it was enabled.  This is a bit of
> +		 * a heavy operation and we disable / enable IRQs a lot, so
> +		 * we'll leave low power mode disabled and it will get
> +		 * re-enabled again in dw_mci_setup_bus().
> +		 */
> +		dw_mci_disable_low_power(mmc);
> +
>  		mci_writel(host, INTMASK,
>  			   (int_mask | SDMMC_INT_SDIO(slot->id)));
>  	} else {
> --
> 1.7.7.3
> 
> --
> 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

--
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
Jaehoon Chung July 24, 2012, 1:36 a.m. UTC | #2 
On 07/24/2012 02:02 AM, Doug Anderson wrote:
> The documentation for the dw_mmc part says that the low power
> mode should normally only be set for MMC and SD memory and should
> be turned off for SDIO cards that need interrupts detected.
> 
> The best place I could find to do this is when the SDIO interrupt
> was first enabled.  I rely on the fact that dw_mci_setup_bus()
> will be called when it's time to reenable.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Commenting fixes requested by Grant Grundler.
> - Be extra certain that we don't re-turn on the low power mode in
>   CLKENA in dw_mci_setup_bus() if SDIO interrupts are enabled.
>   There are no known instances of this happening but it's good to be safe.
> 
>  drivers/mmc/host/dw_mmc.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 72dc3cd..0ab1771 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -627,6 +627,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>  {
>  	struct dw_mci *host = slot->host;
>  	u32 div;
> +	u32 clk_en_a;
>  
>  	if (slot->clock != host->current_speed) {
>  		div = host->bus_hz / slot->clock;
> @@ -659,9 +660,11 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>  		mci_send_cmd(slot,
>  			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>  
> -		/* enable clock */
> -		mci_writel(host, CLKENA, ((SDMMC_CLKEN_ENABLE |
> -			   SDMMC_CLKEN_LOW_PWR) << slot->id));
> +		/* enable clock; only low power if no SDIO */
> +		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
> +		if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id)))
> +			clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
> +		mci_writel(host, CLKENA, clk_en_a);
>  
>  		/* inform CIU */
>  		mci_send_cmd(slot,
> @@ -862,6 +865,32 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	return present;
>  }
>  
> +/*
> + * Disable lower power mode.
> + *
> + * Low power mode will stop the card clock when idle.  According to
> + * documentation (Exynos 5250 User's Manual 0.04, description of
> + * CLKENA register) we should disable low power mode for SDIO cards
> + * if we need interrupts to work.
> + *
As Seungwon is mentioned, that is not exynos5250 specific.
> + * This function is fast if the power mode is already disabled.
> + */
> +static void dw_mci_disable_low_power(struct mmc_host *mmc)
> +{
> +	struct dw_mci_slot *slot = mmc_priv(mmc);
> +	struct dw_mci *host = slot->host;
> +	u32 clk_en_a;
> +	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> +
> +	clk_en_a = mci_readl(host, CLKENA);
> +
> +	if (clk_en_a & clken_low_pwr) {
> +		mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> +		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> +			     SDMMC_CMD_PRV_DAT_WAIT, 0);
> +	}
> +}
> +
>  static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  {
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
> @@ -871,6 +900,14 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>  	/* Enable/disable Slot Specific SDIO interrupt */
>  	int_mask = mci_readl(host, INTMASK);
>  	if (enb) {
> +		/*
> +		 * Turn off low power mode if it was enabled.  This is a bit of
> +		 * a heavy operation and we disable / enable IRQs a lot, so
> +		 * we'll leave low power mode disabled and it will get
> +		 * re-enabled again in dw_mci_setup_bus().
> +		 */
> +		dw_mci_disable_low_power(mmc);
How about using "slot" instead of "mmc"?
> +
>  		mci_writel(host, INTMASK,
>  			   (int_mask | SDMMC_INT_SDIO(slot->id)));
>  	} else {
> 


--
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
Doug Anderson July 24, 2012, 4:58 p.m. UTC | #3 
On Mon, Jul 23, 2012 at 6:17 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> I have followed this patch from v1.
> I think it's a good point.
> Looks good to me.

Thanks for the review!  Looking forward to your ack with the next version.  :)

> Above comment is correct, but this is not specific for Exynos5250.
> Exynos5250 is just one of host controllers base on Synopsys's.
> It'd be better to remove the part related to Exynos you mentioned .

Done.  Grant had requested me to quote exactly which manual I found
the requirement in, but I think describing just that it was found in
the description of the CLKENA register is enough.  I've changed it.

> Definitely, low power mode not power mode, right?

Done.  Good catch!


-Doug
--
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
Doug Anderson July 24, 2012, 4:58 p.m. UTC | #4 
On Mon, Jul 23, 2012 at 6:36 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> How about using "slot" instead of "mmc"?

Done.
--
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/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 72dc3cd..0ab1771 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -627,6 +627,7 @@  static void dw_mci_setup_bus(struct dw_mci_slot *slot)
 {
 	struct dw_mci *host = slot->host;
 	u32 div;
+	u32 clk_en_a;
 
 	if (slot->clock != host->current_speed) {
 		div = host->bus_hz / slot->clock;
@@ -659,9 +660,11 @@  static void dw_mci_setup_bus(struct dw_mci_slot *slot)
 		mci_send_cmd(slot,
 			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
 
-		/* enable clock */
-		mci_writel(host, CLKENA, ((SDMMC_CLKEN_ENABLE |
-			   SDMMC_CLKEN_LOW_PWR) << slot->id));
+		/* enable clock; only low power if no SDIO */
+		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
+		if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id)))
+			clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
+		mci_writel(host, CLKENA, clk_en_a);
 
 		/* inform CIU */
 		mci_send_cmd(slot,
@@ -862,6 +865,32 @@  static int dw_mci_get_cd(struct mmc_host *mmc)
 	return present;
 }
 
+/*
+ * Disable lower power mode.
+ *
+ * Low power mode will stop the card clock when idle.  According to
+ * documentation (Exynos 5250 User's Manual 0.04, description of
+ * CLKENA register) we should disable low power mode for SDIO cards
+ * if we need interrupts to work.
+ *
+ * This function is fast if the power mode is already disabled.
+ */
+static void dw_mci_disable_low_power(struct mmc_host *mmc)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct dw_mci *host = slot->host;
+	u32 clk_en_a;
+	const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+
+	clk_en_a = mci_readl(host, CLKENA);
+
+	if (clk_en_a & clken_low_pwr) {
+		mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
+		mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+			     SDMMC_CMD_PRV_DAT_WAIT, 0);
+	}
+}
+
 static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);
@@ -871,6 +900,14 @@  static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	/* Enable/disable Slot Specific SDIO interrupt */
 	int_mask = mci_readl(host, INTMASK);
 	if (enb) {
+		/*
+		 * Turn off low power mode if it was enabled.  This is a bit of
+		 * a heavy operation and we disable / enable IRQs a lot, so
+		 * we'll leave low power mode disabled and it will get
+		 * re-enabled again in dw_mci_setup_bus().
+		 */
+		dw_mci_disable_low_power(mmc);
+
 		mci_writel(host, INTMASK,
 			   (int_mask | SDMMC_INT_SDIO(slot->id)));
 	} else {