[PATCH 1/3 ] mmc: add support for h/w clock gating of sd controller

[PATCH 1/3 ] mmc: add support for h/w clock gating of sd controller

[Philip Rakity]

This code extends software clock gating in the mmc layer
by adding the ability to indicate that controller support
hardware clock gating.

hardware clock gating is enabled by setting the mmc quirk
MMC_CAP_CLOCK_GATING_HW
in the sd driver.
eg: host->mmc->caps |= MMC_CAP_CLOCK_GATING_HW

The approach follows the suggestion of Nico Pitre.

sd/mmc/eMMC cards use dynmamic clocks
sdio uses continuous clocks

The code has been test using marvell linux for mmp2.  The marvell
controller support h/w clock gating.

Signed-off-by: Philip Rakity <prakity@marvell.com>
---
 drivers/mmc/core/core.c  |   18 +++++++++++++++---
 drivers/mmc/core/core.h  |   19 +++++++++++++++++++
 drivers/mmc/core/host.c  |   10 ++++++++--
 drivers/mmc/core/mmc.c   |    5 +++++
 drivers/mmc/core/sd.c    |    4 ++++
 drivers/mmc/core/sdio.c  |    5 +++++
 include/linux/mmc/host.h |    1 +
 7 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6286898..c8bba7d 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -131,7 +131,10 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 		if (mrq->done)
 			mrq->done(mrq);
 
-		mmc_host_clk_gate(host);
+#ifdef CONFIG_MMC_CLKGATE
+		if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
+			mmc_host_clk_gate(host);
+#endif
 	}
 }
 
@@ -192,7 +195,10 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 			mrq->stop->mrq = mrq;
 		}
 	}
-	mmc_host_clk_ungate(host);
+#ifdef CONFIG_MMC_CLKGATE
+	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
+		mmc_host_clk_ungate(host);
+#endif
 	host->ops->request(host, mrq);
 }
 
@@ -1547,8 +1553,14 @@ void mmc_rescan(struct work_struct *work)
 		pr_info("%s: %s: trying to init card at %u Hz\n",
 			mmc_hostname(host), __func__, host->f_init);
 #endif
-		mmc_power_up(host);
+
+#ifdef CONFIG_MMC_CLKGATE
+		if (host->caps & MMC_CAP_CLOCK_GATING_HW)
+			mmc_hwungate_clock(host);
+#endif
 		sdio_reset(host);
+		mmc_power_up(host);
+
 		mmc_go_idle(host);
 
 		mmc_send_if_cond(host, host->ocr_avail);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 026c975..184d56b 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -35,6 +35,25 @@ void mmc_set_chip_select(struct mmc_host *host, int mode);
 void mmc_set_clock(struct mmc_host *host, unsigned int hz);
 void mmc_gate_clock(struct mmc_host *host);
 void mmc_ungate_clock(struct mmc_host *host);
+
+#ifdef CONFIG_MMC_CLKGATE
+/*
+ * This gates the clock by enabling driver h/w gating
+ */
+static inline void mmc_hwgate_clock(struct mmc_host *host)
+{
+	host->clk_gated = true;
+}
+
+/*
+ * This ungates the clock by turning off h/w gating
+ */
+static inline void mmc_hwungate_clock(struct mmc_host *host)
+{
+	host->clk_gated = false;
+}
+#endif
+
 void mmc_set_ungated(struct mmc_host *host);
 void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode);
 void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 92e3370..ace748e 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -282,7 +282,10 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	host->class_dev.class = &mmc_host_class;
 	device_initialize(&host->class_dev);
 
-	mmc_host_clk_init(host);
+#ifdef CONFIG_MMC_CLKGATE
+	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
+		mmc_host_clk_init(host);
+#endif
 
 	spin_lock_init(&host->lock);
 	init_waitqueue_head(&host->wq);
@@ -366,7 +369,10 @@ void mmc_remove_host(struct mmc_host *host)
 
 	led_trigger_unregister_simple(host->led);
 
-	mmc_host_clk_exit(host);
+#ifdef CONFIG_MMC_CLKGATE
+	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
+		mmc_host_clk_exit(host);
+#endif
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0eccd96..195e557 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -517,6 +517,11 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 
 	mmc_set_clock(host, max_dtr);
 
+#ifdef CONFIG_MMC_CLKGATE
+	if (host->caps & MMC_CAP_CLOCK_GATING_HW)
+		mmc_hwgate_clock(host);
+#endif
+
 	/*
 	 * Indicate DDR mode (if supported).
 	 */
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 49da4df..04e0d6f 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -618,6 +618,10 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	else if (err)
 		goto free_card;
 
+#ifdef CONFIG_MMC_CLKGATE
+	if (host->caps & MMC_CAP_CLOCK_GATING_HW)
+		mmc_hwgate_clock(host);
+#endif
 	/*
 	 * Set bus speed.
 	 */
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index efef5f9..52fbbb7 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -488,6 +488,11 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	else if (err)
 		goto remove;
 
+#ifdef CONFIG_MMC_CLKGATE
+	if (host->caps & MMC_CAP_CLOCK_GATING_HW)
+		mmc_hwungate_clock(host);
+#endif
+
 	/*
 	 * Change to the card's maximum speed.
 	 */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 078cff7..30ce755 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -171,6 +171,7 @@ struct mmc_host {
 #define MMC_CAP_POWER_OFF_CARD	(1 << 13)	/* Can power off after boot */
 
 #define MMC_CAP_BUS_WIDTH_TEST	(1 << 14)	/* CMD14/CMD19 bus width ok */
+#define MMC_CAP_CLOCK_GATING_HW	(1 << 15)	/* h/w supports clock gating */
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
 #ifdef CONFIG_MMC_CLKGATE
-- 
1.6.0.4

Re: [PATCH 1/3 ] mmc: add support for h/w clock gating of sd controller

[Nicolas Pitre]

On Fri, 3 Dec 2010, Philip Rakity wrote:

> This code extends software clock gating in the mmc layer
> by adding the ability to indicate that controller support
> hardware clock gating.
> 
> hardware clock gating is enabled by setting the mmc quirk
> MMC_CAP_CLOCK_GATING_HW
> in the sd driver.
> eg: host->mmc->caps |= MMC_CAP_CLOCK_GATING_HW
> 
> The approach follows the suggestion of Nico Pitre.
> 
> sd/mmc/eMMC cards use dynmamic clocks
> sdio uses continuous clocks
> 
> The code has been test using marvell linux for mmp2.  The marvell
> controller support h/w clock gating.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>

Comments below.

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6286898..c8bba7d 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -131,7 +131,10 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>  		if (mrq->done)
>  			mrq->done(mrq);
>  
> -		mmc_host_clk_gate(host);
> +#ifdef CONFIG_MMC_CLKGATE
> +		if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
> +			mmc_host_clk_gate(host);
> +#endif

This is a needless proliferation of #ifdef's, since you could just test 
for MMC_CAP_CLOCK_GATING_HW inside mmc_host_clk_gate() instead, and 
return early if set.

>  }
>  
> @@ -192,7 +195,10 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>  			mrq->stop->mrq = mrq;
>  		}
>  	}
> -	mmc_host_clk_ungate(host);
> +#ifdef CONFIG_MMC_CLKGATE
> +	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
> +		mmc_host_clk_ungate(host);
> +#endif

Same thing here.

>  	host->ops->request(host, mrq);
>  }
>  
> @@ -1547,8 +1553,14 @@ void mmc_rescan(struct work_struct *work)
>  		pr_info("%s: %s: trying to init card at %u Hz\n",
>  			mmc_hostname(host), __func__, host->f_init);
>  #endif
> -		mmc_power_up(host);
> +
> +#ifdef CONFIG_MMC_CLKGATE
> +		if (host->caps & MMC_CAP_CLOCK_GATING_HW)
> +			mmc_hwungate_clock(host);
> +#endif

Here you should do just like what is done for mmc_host_clk_gate() in 
host.h: provide an empty inline version when CONFIG_MMC_CLKGATE is not 
defined and get rid of the #ifdef here.

>  		sdio_reset(host);
> +		mmc_power_up(host);

Why did you move down this mmc_power_up()?

>  		mmc_go_idle(host);
>  
>  		mmc_send_if_cond(host, host->ocr_avail);
[...]
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 92e3370..ace748e 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -282,7 +282,10 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>  	host->class_dev.class = &mmc_host_class;
>  	device_initialize(&host->class_dev);
>  
> -	mmc_host_clk_init(host);
> +#ifdef CONFIG_MMC_CLKGATE
> +	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
> +		mmc_host_clk_init(host);
> +#endif

Same comment as above: why don't you check for MMC_CAP_CLOCK_GATING_HW 
inside mmc_host_clk_init()?

Granted, here mmc_host_clk_init() appears to be misnomed (maybe 
mmc_host_clk_gating_init() would have been clearer).

>  	spin_lock_init(&host->lock);
>  	init_waitqueue_head(&host->wq);
> @@ -366,7 +369,10 @@ void mmc_remove_host(struct mmc_host *host)
>  
>  	led_trigger_unregister_simple(host->led);
>  
> -	mmc_host_clk_exit(host);
> +#ifdef CONFIG_MMC_CLKGATE
> +	if ((host->caps & MMC_CAP_CLOCK_GATING_HW) == 0)
> +		mmc_host_clk_exit(host);
> +#endif

Ditto here.

>  }
>  
>  EXPORT_SYMBOL(mmc_remove_host);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0eccd96..195e557 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -517,6 +517,11 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>  
>  	mmc_set_clock(host, max_dtr);
>  
> +#ifdef CONFIG_MMC_CLKGATE
> +	if (host->caps & MMC_CAP_CLOCK_GATING_HW)
> +		mmc_hwgate_clock(host);
> +#endif

And here I seriously begin to dislike this patch.  The sw gating code is 
centralized while the hw one is spread across all card type drivers.  
Why not calling mmc_hwgate_clock() at the appropriate location within 
mmc_rescan() (that would be where it is determined that only anew card 
might be present) and call mmc_hwgate_clock() at the end of mmc_rescan() 
and only if mmc_host_may_gate_card() is true?


Nicolas

Re: [PATCH 1/3 ] mmc: add support for h/w clock gating of sd controller

[Wolfram Sang]

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

Hi,

minor comments in addition to Nicolas:

On Fri, Dec 03, 2010 at 11:26:50AM -0800, Philip Rakity wrote:

> hardware clock gating is enabled by setting the mmc quirk

It's not a quirk, it's a capability :)

> MMC_CAP_CLOCK_GATING_HW

Why not MMC_CAP_HW_CLOCK_GATING? Might be a mileage, though...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 1/3 ] mmc: add support for h/w clock gating of sd controller

[Philip Rakity]

will change tomorrow with other changes Nico Suggested.

Philip

On Dec 4, 2010, at 1:02 PM, Wolfram Sang wrote:

> Hi,
> 
> minor comments in addition to Nicolas:
> 
> On Fri, Dec 03, 2010 at 11:26:50AM -0800, Philip Rakity wrote:
> 
>> hardware clock gating is enabled by setting the mmc quirk
> 
> It's not a quirk, it's a capability :)
> 
>> MMC_CAP_CLOCK_GATING_HW
> 
> Why not MMC_CAP_HW_CLOCK_GATING? Might be a mileage, though...
> 
> Regards,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |