[PATCH 1/2] mmc: agressive clocking framework v8

[PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

This patch modifies the MMC core code to optionally call the
set_ios() operation on the driver with the clock frequency set
to 0 (gate) after a grace period of at least 8 MCLK cycles, then
restore it (ungate) before any new request. This gives
the driver the option to shut down the MCI clock to the MMC/SD
card when the clock frequency is 0, i.e. the core has stated
that the MCI clock does not need to be generated.

It is inspired by existing clock gating code found in the OMAP
and Atmel drivers and brings this up to the host abstraction.
Gating is performed before and after any MMC request.

It exemplifies by implementing this for the MMCI/PL180 MMC/SD
host controller, but it should be simple to switch OMAP and
Atmel over to using this instead.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
Changes v7->v8: introduce a check so we don't attempt to
gate off SDIO cards for now. Those who want their SDIO cards
gated can continue from here the day they want.
---
 drivers/mmc/core/Kconfig   |   11 +++
 drivers/mmc/core/core.c    |   45 +++++++++-
 drivers/mmc/core/core.h    |    2 +
 drivers/mmc/core/debugfs.c |    5 +
 drivers/mmc/core/host.c    |  207 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/core/host.h    |   21 +++++
 include/linux/mmc/host.h   |   10 ++
 7 files changed, 299 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index bb22ffd..1018ccc1 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -16,3 +16,14 @@ config MMC_UNSAFE_RESUME
 
 	  This option sets a default which can be overridden by the
 	  module parameter "removable=0" or "removable=1".
+
+config MMC_CLKGATE
+	bool "MMC host clock gating (EXPERIMENTAL)"
+	depends on EXPERIMENTAL
+	help
+	  This will attempt to agressively gate the clock to the MMC card.
+	  This is done to save power due to gating off the logic and bus
+	  noise when the MMC card is not in use. Your host driver has to
+	  support handling this in order for it to be of any use.
+
+	  If unsure, say N.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8f86d70..1be5d65 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -130,6 +130,8 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 
 		if (mrq->done)
 			mrq->done(mrq);
+
+		mmc_host_clk_gate(host);
 	}
 }
 
@@ -190,6 +192,7 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 			mrq->stop->mrq = mrq;
 		}
 	}
+	mmc_host_clk_ungate(host);
 	host->ops->request(host, mrq);
 }
 
@@ -296,7 +299,7 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 
 		timeout_us = data->timeout_ns / 1000;
 		timeout_us += data->timeout_clks * 1000 /
-			(card->host->ios.clock / 1000);
+			(mmc_host_clk_rate(card->host) / 1000);
 
 		if (data->flags & MMC_DATA_WRITE)
 			/*
@@ -614,6 +617,12 @@ static inline void mmc_set_ios(struct mmc_host *host)
 		 ios->power_mode, ios->chip_select, ios->vdd,
 		 ios->bus_width, ios->timing);
 
+	/*
+	 * We get a new frequency while the clock is gated,
+	 * so make sure we regard this as ungating it.
+	 */
+	if (ios->clock > 0 && host->clk_gated)
+		host->clk_gated = false;
 	host->ops->set_ios(host, ios);
 }
 
@@ -641,6 +650,40 @@ void mmc_set_clock(struct mmc_host *host, unsigned int hz)
 	mmc_set_ios(host);
 }
 
+#ifdef CONFIG_MMC_CLKGATE
+/*
+ * This gates the clock by setting it to 0 Hz.
+ */
+void mmc_gate_clock(struct mmc_host *host)
+{
+	host->clk_old = host->ios.clock;
+	host->ios.clock = 0;
+	host->clk_gated = true;
+	mmc_set_ios(host);
+}
+
+/*
+ * This restores the clock from gating by using the cached
+ * clock value.
+ */
+void mmc_ungate_clock(struct mmc_host *host)
+{
+	/*
+	 * We should previously have gated the clock, so the clock
+	 * shall be 0 here!
+	 * The clock may however be 0 during intialization,
+	 * when some request operations are performed before setting
+	 * the frequency. When ungate is requested in that situation
+	 * we just ignore the call.
+	 */
+	if (host->clk_old) {
+		BUG_ON(host->ios.clock);
+		mmc_set_clock(host, host->clk_old);
+	}
+	host->clk_gated = false;
+}
+#endif
+
 /*
  * Change the bus mode (open drain/push-pull) of a host.
  */
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 77240cd..9972808 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -33,6 +33,8 @@ void mmc_init_erase(struct mmc_card *card);
 
 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);
 void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode);
 void mmc_set_bus_width(struct mmc_host *host, unsigned int width);
 void mmc_set_bus_width_ddr(struct mmc_host *host, unsigned int width,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index eed1405..998797e 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -183,6 +183,11 @@ void mmc_add_host_debugfs(struct mmc_host *host)
 			&mmc_clock_fops))
 		goto err_node;
 
+#ifdef CONFIG_MMC_CLKGATE
+	if (!debugfs_create_u32("clk_delay", (S_IRUSR | S_IWUSR),
+				root, &host->clk_delay))
+		goto err_node;
+#endif
 	return;
 
 err_node:
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 10b8af2..958409a 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2003 Russell King, All Rights Reserved.
  *  Copyright (C) 2007-2008 Pierre Ossman
+ *  Copyright (C) 2010 Linus Walleij
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -20,6 +21,7 @@
 #include <linux/suspend.h>
 
 #include <linux/mmc/host.h>
+#include <linux/mmc/card.h>
 
 #include "core.h"
 #include "host.h"
@@ -50,6 +52,206 @@ void mmc_unregister_host_class(void)
 static DEFINE_IDR(mmc_host_idr);
 static DEFINE_SPINLOCK(mmc_host_lock);
 
+#ifdef CONFIG_MMC_CLKGATE
+
+/*
+ * Enabling clock gating will make the core call out to the host
+ * once up and once down when it performs a request or card operation
+ * intermingled in any fashion. The driver will see this through
+ * set_ios() operations with ios.clock field set to 0 to gate
+ * (disable) the block clock, and to the old frequency to enable
+ * it again.
+ */
+static void mmc_host_clk_gate_delayed(struct mmc_host *host)
+{
+	unsigned long tick_ns;
+	unsigned long freq = host->ios.clock;
+	unsigned long flags;
+	int users;
+
+	if (!freq) {
+		pr_err("%s: frequency set to 0 in disable function, "
+		       "this means the clock is already disabled.\n",
+		       mmc_hostname(host));
+		return;
+	}
+	/*
+	 * New requests may have appeared while we were scheduling,
+	 * then there is no reason to delay the check before
+	 * clk_disable().
+	 */
+	spin_lock_irqsave(&host->clk_lock, flags);
+	users = host->clk_requests;
+	/*
+	 * Delay n bus cycles (at least 8 from MMC spec) before attempting
+	 * to disable the MCI block clock. The reference count
+	 * may have gone up again after this delay due to
+	 * rescheduling!
+	 */
+	if (!users) {
+		spin_unlock_irqrestore(&host->clk_lock, flags);
+		tick_ns = DIV_ROUND_UP(1000000000, freq);
+		ndelay(host->clk_delay * tick_ns);
+	} else {
+		/* New users appeared while waiting for this work */
+		host->clk_pending_gate = false;
+		spin_unlock_irqrestore(&host->clk_lock, flags);
+		return;
+	}
+	spin_lock_irqsave(&host->clk_lock, flags);
+	if (!host->clk_requests) {
+		spin_unlock_irqrestore(&host->clk_lock, flags);
+		/* this will set host->ios.clock to 0 */
+		mmc_gate_clock(host);
+		spin_lock_irqsave(&host->clk_lock, flags);
+		pr_debug("%s: gated MCI clock\n",
+			 mmc_hostname(host));
+	}
+	host->clk_pending_gate = false;
+	spin_unlock_irqrestore(&host->clk_lock, flags);
+}
+
+/*
+ * Internal work. Work to disable the clock at some later point.
+ */
+static void mmc_host_clk_gate_work(struct work_struct *work)
+{
+	struct mmc_host *host = container_of(work, struct mmc_host,
+					      clk_disable_work);
+
+	mmc_host_clk_gate_delayed(host);
+}
+
+/*
+ *	mmc_host_clk_ungate - make sure the host ios.clock is
+ *	restored to some non-zero value past this call.
+ *	@host: host to ungate.
+ *
+ *	Increase clock reference count and ungate clock if first user.
+ */
+void mmc_host_clk_ungate(struct mmc_host *host)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->clk_lock, flags);
+	if (host->clk_gated) {
+		spin_unlock_irqrestore(&host->clk_lock, flags);
+		mmc_ungate_clock(host);
+		spin_lock_irqsave(&host->clk_lock, flags);
+		pr_debug("%s: ungated MCI clock\n",
+			 mmc_hostname(host));
+	}
+	host->clk_requests++;
+	spin_unlock_irqrestore(&host->clk_lock, flags);
+}
+
+/*
+ *	mmc_host_may_gate_card - check if this card may be gated
+ *	@card: card to check.
+ */
+static bool mmc_host_may_gate_card(struct mmc_card *card)
+{
+	/* If there is no card we may gate it */
+	if (!card)
+		return true;
+	/*
+	 * Don't gate SDIO cards! These need to be clocked at all times
+	 * since they may be independent systems generating interrupts
+	 * and other events. The clock requests counter from the core will
+	 * go down to zero since the core does not need it, but we will not
+	 * gate the clock, because there is somebody out there that may still
+	 * be using it.
+	 */
+	if (mmc_card_sdio(card))
+		return false;
+
+	return true;
+}
+
+/*
+ *	mmc_host_clk_gate - call the host driver with ios.clock
+ *	set to zero as often as possible so as to make it
+ *	possible to gate off hardware MCI clocks.
+ *	@host: host to gate.
+ *
+ *	Decrease clock reference count and schedule disablement of clock.
+ */
+void mmc_host_clk_gate(struct mmc_host *host)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->clk_lock, flags);
+	host->clk_requests--;
+	if (mmc_host_may_gate_card(host->card) &&
+	    !host->clk_requests) {
+		host->clk_pending_gate = true;
+		schedule_work(&host->clk_disable_work);
+	}
+	spin_unlock_irqrestore(&host->clk_lock, flags);
+}
+
+/*
+ *	mmc_host_clk_rate - get current clock frequency setting no matter
+ *	whether it's gated or not.
+ *	@host: host to get the clock frequency for.
+ */
+unsigned int mmc_host_clk_rate(struct mmc_host *host)
+{
+	unsigned long freq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&host->clk_lock, flags);
+	if (host->clk_gated)
+		freq = host->clk_old;
+	else
+		freq = host->ios.clock;
+	spin_unlock_irqrestore(&host->clk_lock, flags);
+	return freq;
+}
+
+/*
+ *	mmc_host_clk_init - set up clock gating code
+ *	@host: host with potential clock to control
+ */
+static inline void mmc_host_clk_init(struct mmc_host *host)
+{
+	host->clk_requests = 0;
+	host->clk_delay = 8; /* hold MCI clock in 8 cycles by default */
+	host->clk_gated = false;
+	host->clk_pending_gate = false;
+	INIT_WORK(&host->clk_disable_work, mmc_host_clk_gate_work);
+	spin_lock_init(&host->clk_lock);
+}
+
+/*
+ *	mmc_host_clk_exit - shut down clock gating code
+ *	@host: host with potential clock to control
+ */
+static inline void mmc_host_clk_exit(struct mmc_host *host)
+{
+	/*
+	 * Wait for any outstanding gate and then make sure we're
+	 * ungated before exiting.
+	 */
+	if (cancel_work_sync(&host->clk_disable_work))
+		mmc_host_clk_gate_delayed(host);
+	if (host->clk_gated)
+		mmc_host_clk_ungate(host);
+	BUG_ON(host->clk_requests > 0);
+}
+
+#else
+
+static inline void mmc_host_clk_init(struct mmc_host *host)
+{
+}
+
+static inline void mmc_host_clk_exit(struct mmc_host *host)
+{
+}
+
+#endif
+
 /**
  *	mmc_alloc_host - initialise the per-host structure.
  *	@extra: sizeof private data structure
@@ -82,6 +284,8 @@ 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);
+
 	spin_lock_init(&host->lock);
 	init_waitqueue_head(&host->wq);
 	INIT_DELAYED_WORK(&host->detect, mmc_rescan);
@@ -163,6 +367,8 @@ void mmc_remove_host(struct mmc_host *host)
 	device_del(&host->class_dev);
 
 	led_trigger_unregister_simple(host->led);
+
+	mmc_host_clk_exit(host);
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
@@ -183,4 +389,3 @@ void mmc_free_host(struct mmc_host *host)
 }
 
 EXPORT_SYMBOL(mmc_free_host);
-
diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
index 8c87e11..de199f9 100644
--- a/drivers/mmc/core/host.h
+++ b/drivers/mmc/core/host.h
@@ -10,10 +10,31 @@
  */
 #ifndef _MMC_CORE_HOST_H
 #define _MMC_CORE_HOST_H
+#include <linux/mmc/host.h>
 
 int mmc_register_host_class(void);
 void mmc_unregister_host_class(void);
 
+#ifdef CONFIG_MMC_CLKGATE
+void mmc_host_clk_ungate(struct mmc_host *host);
+void mmc_host_clk_gate(struct mmc_host *host);
+unsigned int mmc_host_clk_rate(struct mmc_host *host);
+
+#else
+static inline void mmc_host_clk_ungate(struct mmc_host *host)
+{
+}
+
+static inline void mmc_host_clk_gate(struct mmc_host *host)
+{
+}
+
+static inline unsigned int mmc_host_clk_rate(struct mmc_host *host)
+{
+	return host->ios.clock;
+}
+#endif
+
 void mmc_host_deeper_disable(struct work_struct *work);
 
 #endif
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 6d87f68..c38c400 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -171,6 +171,16 @@ struct mmc_host {
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
+#ifdef CONFIG_MMC_CLKGATE
+	int			clk_requests;	/* internal reference counter */
+	unsigned int		clk_delay;	/* number of MCI clk hold cycles */
+	bool			clk_gated;	/* clock gated */
+	bool			clk_pending_gate; /* pending clock gating */
+	struct work_struct	clk_disable_work; /* delayed clock disablement */
+	unsigned int		clk_old;	/* old clock value cache */
+	spinlock_t		clk_lock;	/* lock for clk fields */
+#endif
+
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
 	unsigned short		max_segs;	/* see blk_queue_max_segments */
-- 
1.7.2.3

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

2010/11/3 Linus Walleij <linus.walleij@stericsson.com>:

> This patch modifies the MMC core code to optionally call the
> set_ios() operation on the driver with the clock frequency set
> to 0 (gate) after a grace period of at least 8 MCLK cycles, then
> restore it (ungate) before any new request. This gives
> the driver the option to shut down the MCI clock to the MMC/SD
> card when the clock frequency is 0, i.e. the core has stated
> that the MCI clock does not need to be generated.
>
> It is inspired by existing clock gating code found in the OMAP
> and Atmel drivers and brings this up to the host abstraction.
> Gating is performed before and after any MMC request.
>
> It exemplifies by implementing this for the MMCI/PL180 MMC/SD
> host controller, but it should be simple to switch OMAP and
> Atmel over to using this instead.
>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>

I think we have some rough consesus that:

* This patch adds the desired gating functionality to the MMC
  subsystem.
* It can be refactored to reuse parts of runtime PM in the
  future if some eager contributor turns up.

Andrew, can you add this v8 version (not the misguided v9!)
version to your patch stack?

Yours,
Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

2010/11/8 Linus Walleij <linus.ml.walleij@gmail.com>:

> Andrew, can you add this v8 version (not the misguided v9!)
> version to your patch stack?

I'm slow in the head. We have an MMC maintainer now.
Chris, can you add v8 to your for-next git?

Yours,
Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

Hi Linus,

On Mon, Nov 08, 2010 at 11:30:41PM +0100, Linus Walleij wrote:
> Chris, can you add v8 to your for-next git?

Done -- thanks very much for doing this!  I fixed up a compile error
when CONFIG_MMC_CLKGATE=n:

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 746730e..8bf542c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -617,12 +617,15 @@ static inline void mmc_set_ios(struct mmc_host *host)
 		 ios->power_mode, ios->chip_select, ios->vdd,
 		 ios->bus_width, ios->timing);
 
+#ifdef CONFIG_MMC_CLKGATE
 	/*
 	 * We've been given a new frequency while the clock is gated,
 	 * so make sure we regard this as ungating it.
 	 */
 	if (ios->clock > 0 && host->clk_gated)
 		host->clk_gated = false;
+#endif
+
 	host->ops->set_ios(host, ios);
 }
 
.. and I also made trivial stylistic changes, let me know if you
disagree with anything below.  ("/**" is a required preamble for
kerneldoc comments.)

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index 1018ccc1..ef10387 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -21,7 +21,7 @@ config MMC_CLKGATE
 	bool "MMC host clock gating (EXPERIMENTAL)"
 	depends on EXPERIMENTAL
 	help
-	  This will attempt to agressively gate the clock to the MMC card.
+	  This will attempt to aggressively gate the clock to the MMC card.
 	  This is done to save power due to gating off the logic and bus
 	  noise when the MMC card is not in use. Your host driver has to
 	  support handling this in order for it to be of any use.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 1be5d65..746730e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -618,7 +618,7 @@ static inline void mmc_set_ios(struct mmc_host *host)
 		 ios->bus_width, ios->timing);
 
 	/*
-	 * We get a new frequency while the clock is gated,
+	 * We've been given a new frequency while the clock is gated,
 	 * so make sure we regard this as ungating it.
 	 */
 	if (ios->clock > 0 && host->clk_gated)
@@ -669,9 +669,8 @@ void mmc_gate_clock(struct mmc_host *host)
 void mmc_ungate_clock(struct mmc_host *host)
 {
 	/*
-	 * We should previously have gated the clock, so the clock
-	 * shall be 0 here!
-	 * The clock may however be 0 during intialization,
+	 * We should previously have gated the clock, so the clock shall
+	 * be 0 here! The clock may however be 0 during initialization,
 	 * when some request operations are performed before setting
 	 * the frequency. When ungate is requested in that situation
 	 * we just ignore the call.
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 958409a..aacd9c5 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -58,9 +58,8 @@ static DEFINE_SPINLOCK(mmc_host_lock);
  * Enabling clock gating will make the core call out to the host
  * once up and once down when it performs a request or card operation
  * intermingled in any fashion. The driver will see this through
- * set_ios() operations with ios.clock field set to 0 to gate
- * (disable) the block clock, and to the old frequency to enable
- * it again.
+ * set_ios() operations with ios.clock field set to 0 to gate (disable)
+ * the block clock, and to the old frequency to enable it again.
  */
 static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 {
@@ -82,11 +81,11 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 	 */
 	spin_lock_irqsave(&host->clk_lock, flags);
 	users = host->clk_requests;
+
 	/*
 	 * Delay n bus cycles (at least 8 from MMC spec) before attempting
-	 * to disable the MCI block clock. The reference count
-	 * may have gone up again after this delay due to
-	 * rescheduling!
+	 * to disable the MCI block clock. The reference count may have
+	 * gone up again after this delay due to rescheduling!
 	 */
 	if (!users) {
 		spin_unlock_irqrestore(&host->clk_lock, flags);
@@ -101,11 +100,10 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 	spin_lock_irqsave(&host->clk_lock, flags);
 	if (!host->clk_requests) {
 		spin_unlock_irqrestore(&host->clk_lock, flags);
-		/* this will set host->ios.clock to 0 */
+		/* This will set host->ios.clock to 0 */
 		mmc_gate_clock(host);
 		spin_lock_irqsave(&host->clk_lock, flags);
-		pr_debug("%s: gated MCI clock\n",
-			 mmc_hostname(host));
+		pr_debug("%s: gated MCI clock\n", mmc_hostname(host));
 	}
 	host->clk_pending_gate = false;
 	spin_unlock_irqrestore(&host->clk_lock, flags);
@@ -122,12 +120,13 @@ static void mmc_host_clk_gate_work(struct work_struct *work)
 	mmc_host_clk_gate_delayed(host);
 }
 
-/*
- *	mmc_host_clk_ungate - make sure the host ios.clock is
- *	restored to some non-zero value past this call.
+/**
+ *	mmc_host_clk_ungate - ungate hardware MCI clocks
  *	@host: host to ungate.
  *
- *	Increase clock reference count and ungate clock if first user.
+ *	Makes sure the host ios.clock is restored to a non-zero value
+ *	past this call.	Increase clock reference count and ungate clock
+ *	if we're the first user.
  */
 void mmc_host_clk_ungate(struct mmc_host *host)
 {
@@ -138,14 +137,13 @@ void mmc_host_clk_ungate(struct mmc_host *host)
 		spin_unlock_irqrestore(&host->clk_lock, flags);
 		mmc_ungate_clock(host);
 		spin_lock_irqsave(&host->clk_lock, flags);
-		pr_debug("%s: ungated MCI clock\n",
-			 mmc_hostname(host));
+		pr_debug("%s: ungated MCI clock\n", mmc_hostname(host));
 	}
 	host->clk_requests++;
 	spin_unlock_irqrestore(&host->clk_lock, flags);
 }
 
-/*
+/**
  *	mmc_host_may_gate_card - check if this card may be gated
  *	@card: card to check.
  */
@@ -168,13 +166,13 @@ static bool mmc_host_may_gate_card(struct mmc_card *card)
 	return true;
 }
 
-/*
- *	mmc_host_clk_gate - call the host driver with ios.clock
- *	set to zero as often as possible so as to make it
- *	possible to gate off hardware MCI clocks.
+/**
+ *	mmc_host_clk_gate - gate off hardware MCI clocks
  *	@host: host to gate.
  *
- *	Decrease clock reference count and schedule disablement of clock.
+ *	Calls the host driver with ios.clock set to zero as often as possible
+ *	in order to gate off hardware MCI clocks. Decrease clock reference
+ *	count and schedule disabling of clock.
  */
 void mmc_host_clk_gate(struct mmc_host *host)
 {
@@ -190,10 +188,11 @@ void mmc_host_clk_gate(struct mmc_host *host)
 	spin_unlock_irqrestore(&host->clk_lock, flags);
 }
 
-/*
- *	mmc_host_clk_rate - get current clock frequency setting no matter
- *	whether it's gated or not.
+/**
+ *	mmc_host_clk_rate - get current clock frequency setting
  *	@host: host to get the clock frequency for.
+ *
+ *	Returns current clock frequency regardless of gating.
  */
 unsigned int mmc_host_clk_rate(struct mmc_host *host)
 {
@@ -209,21 +208,22 @@ unsigned int mmc_host_clk_rate(struct mmc_host *host)
 	return freq;
 }
 
-/*
+/**
  *	mmc_host_clk_init - set up clock gating code
  *	@host: host with potential clock to control
  */
 static inline void mmc_host_clk_init(struct mmc_host *host)
 {
 	host->clk_requests = 0;
-	host->clk_delay = 8; /* hold MCI clock in 8 cycles by default */
+	/* Hold MCI clock for 8 cycles by default */
+	host->clk_delay = 8;
 	host->clk_gated = false;
 	host->clk_pending_gate = false;
 	INIT_WORK(&host->clk_disable_work, mmc_host_clk_gate_work);
 	spin_lock_init(&host->clk_lock);
 }
 
-/*
+/**
  *	mmc_host_clk_exit - shut down clock gating code
  *	@host: host with potential clock to control
  */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c38c400..f108cee 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -176,7 +176,7 @@ struct mmc_host {
 	unsigned int		clk_delay;	/* number of MCI clk hold cycles */
 	bool			clk_gated;	/* clock gated */
 	bool			clk_pending_gate; /* pending clock gating */
-	struct work_struct	clk_disable_work; /* delayed clock disablement */
+	struct work_struct	clk_disable_work; /* delayed clock disable */
 	unsigned int		clk_old;	/* old clock value cache */
 	spinlock_t		clk_lock;	/* lock for clk fields */
 #endif


-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

Chris Ball wrote:

> Done -- thanks very much for doing this!  I fixed up a compile error
> when CONFIG_MMC_CLKGATE=n:

Thanks for the cleanup Chris, let's see if this one
holds now then... :-)

Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Ohad Ben-Cohen]

Hi Linus,

On Wed, Nov 3, 2010 at 11:22 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
...
> +static void mmc_host_clk_gate_delayed(struct mmc_host *host)
> +{
> +       unsigned long tick_ns;
> +       unsigned long freq = host->ios.clock;
> +       unsigned long flags;
> +       int users;
> +
> +       if (!freq) {
> +               pr_err("%s: frequency set to 0 in disable function, "
> +                      "this means the clock is already disabled.\n",
> +                      mmc_hostname(host));
> +               return;
> +       }
> +       /*
> +        * New requests may have appeared while we were scheduling,
> +        * then there is no reason to delay the check before
> +        * clk_disable().
> +        */
> +       spin_lock_irqsave(&host->clk_lock, flags);
> +       users = host->clk_requests;
> +       /*
> +        * Delay n bus cycles (at least 8 from MMC spec) before attempting
> +        * to disable the MCI block clock. The reference count
> +        * may have gone up again after this delay due to
> +        * rescheduling!
> +        */
> +       if (!users) {
> +               spin_unlock_irqrestore(&host->clk_lock, flags);
> +               tick_ns = DIV_ROUND_UP(1000000000, freq);
> +               ndelay(host->clk_delay * tick_ns);
> +       } else {
> +               /* New users appeared while waiting for this work */
> +               host->clk_pending_gate = false;
> +               spin_unlock_irqrestore(&host->clk_lock, flags);
> +               return;
> +       }
> +       spin_lock_irqsave(&host->clk_lock, flags);
> +       if (!host->clk_requests) {
> +               spin_unlock_irqrestore(&host->clk_lock, flags);

What if mmc_host_clk_ungate() is invoked (and completely executes) at
this point (as a result of a new mmc request) ?

> +               /* this will set host->ios.clock to 0 */
> +               mmc_gate_clock(host);

Will this clock gating not disrupt that new mmc request ?

Or am I missing something ?

> +               spin_lock_irqsave(&host->clk_lock, flags);
> +               pr_debug("%s: gated MCI clock\n",
> +                        mmc_hostname(host));
> +       }
> +       host->clk_pending_gate = false;

What is clk_pending_gate used for (I can only see it being assigned values) ?

Thanks,
Ohad.

(PS sorry for the belated posting of these questions)

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

Ohad Ben-Cohen wrote:

> Hi Linus,
> On Wed, Nov 3, 2010 at 11:22 AM, Linus Walleij
> <linus.walleij@stericsson.com> wrote:
> ...
>> +static void mmc_host_clk_gate_delayed(struct mmc_host *host)
>> +{
>> +       unsigned long tick_ns;
>> +       unsigned long freq = host->ios.clock;
>> +       unsigned long flags;
>> +       int users;
>> +
>> +       if (!freq) {
>> +               pr_err("%s: frequency set to 0 in disable function, "
>> +                      "this means the clock is already disabled.\n",
>> +                      mmc_hostname(host));
>> +               return;
>> +       }
>> +       /*
>> +        * New requests may have appeared while we were scheduling,
>> +        * then there is no reason to delay the check before
>> +        * clk_disable().
>> +        */
>> +       spin_lock_irqsave(&host->clk_lock, flags);
>> +       users = host->clk_requests;
>> +       /*
>> +        * Delay n bus cycles (at least 8 from MMC spec) before attempting
>> +        * to disable the MCI block clock. The reference count
>> +        * may have gone up again after this delay due to
>> +        * rescheduling!
>> +        */
>> +       if (!users) {

Note to self: remove the local users variable and look directly
at host->clk_requests.

>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
>> +               tick_ns = DIV_ROUND_UP(1000000000, freq);
>> +               ndelay(host->clk_delay * tick_ns);
>> +       } else {
>> +               /* New users appeared while waiting for this work */
>> +               host->clk_pending_gate = false;
>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
>> +               return;
>> +       }
>> +       spin_lock_irqsave(&host->clk_lock, flags);
>> +       if (!host->clk_requests) {
>> +               spin_unlock_irqrestore(&host->clk_lock, flags);
> 
> What if mmc_host_clk_ungate() is invoked (and completely executes) at
> this point (as a result of a new mmc request) ?
> 
>> +               /* this will set host->ios.clock to 0 */
>> +               mmc_gate_clock(host);
> 
> Will this clock gating not disrupt that new mmc request ?

Not that one, because the only place where ungate is called
is immediately before the request or set_ios(). So the request
or set_ios() will complete, and immediately after that
this gating will be triggered.

So the real bug is that if we get this race we don't get
the 8 MCI cycles of delay that we want.

But I guess I can replace all spinlocks with a mutex instead
and still hold it across the gate operation, since all
gate/ungate calls should be coming from process context?

That simplifies things.

I'll see if this works...

>> +               spin_lock_irqsave(&host->clk_lock, flags);
>> +               pr_debug("%s: gated MCI clock\n",
>> +                        mmc_hostname(host));
>> +       }
>> +       host->clk_pending_gate = false;
> 
> What is clk_pending_gate used for (I can only see it being assigned values) ?

Hm, a development artifact from patchset v4 2009-06-18...
It's replaced with host->clk_gated instead.

I'll remove it.

> (PS sorry for the belated posting of these questions)

No problem, I'll fix.

Chris, do you want an incremental patch or shall I spin an all-new
v10 patch?

Yours,
Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

Hi Linus,

On Wed, Nov 10, 2010 at 05:25:43PM +0100, Linus Walleij wrote:
> Chris, do you want an incremental patch or shall I spin an all-new
> v10 patch?

I think incremental patches make sense from here on out, now that it's
in -next.  We can decide whether it's worth performing any rebasing
once the patch is looking complete, before it goes into mainline. 

Thanks!

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Ohad Ben-Cohen]

On Wed, Nov 10, 2010 at 6:25 PM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> Not that one, because the only place where ungate is called
> is immediately before the request or set_ios(). So the request
> or set_ios() will complete

How can you assume that ? Nothing prevents a context switch after
ungate, and before the actual request.

Here is what I had in mind:

Thread 1
=======

calling mmc_host_clk_gate_delayed()
...
spin_unlock_irqrestore(&host->clk_lock, flags);

About to execute mmc_gate_clock(host)

context switch to Thread 2
=====================

calling mmc_start_request()
...
executes mmc_host_clk_ungate();

back to Thread 1
=============

executes mmc_gate_clock(host);


back to Thread 2
==============

/* clock is gated now */
host->ops->request(host, mrq);


>, and immediately after that
> this gating will be triggered.
>
> So the real bug is that if we get this race we don't get
> the 8 MCI cycles of delay that we want.
>
> But I guess I can replace all spinlocks with a mutex instead
> and still hold it across the gate operation, since all
> gate/ungate calls should be coming from process context?
>
> That simplifies things.
>
> I'll see if this works...
>
>>> +               spin_lock_irqsave(&host->clk_lock, flags);
>>> +               pr_debug("%s: gated MCI clock\n",
>>> +                        mmc_hostname(host));
>>> +       }
>>> +       host->clk_pending_gate = false;
>>
>> What is clk_pending_gate used for (I can only see it being assigned values) ?
>
> Hm, a development artifact from patchset v4 2009-06-18...
> It's replaced with host->clk_gated instead.
>
> I'll remove it.
>
>> (PS sorry for the belated posting of these questions)
>
> No problem, I'll fix.
>
> Chris, do you want an incremental patch or shall I spin an all-new
> v10 patch?
>
> Yours,
> Linus Walleij
>

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

Hi Linus,

Trying out CLKGATE=y on my x86 laptop with sdhci-pci:

<boot, slots are empty>

[   10.080469] Registered led device: mmc0::
[   10.080595] mmc0: SDHCI controller on PCI [0000:0d:00.0] using DMA
[   10.107996] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[   10.122164] Registered led device: mmc1::
[   10.122237] mmc1: SDHCI controller on PCI [0000:17:00.0] using DMA
[   10.136067] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[   10.164089] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[   10.192076] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[   10.291095] mmc1: frequency set to 0 in disable function, this means the clock is already disabled.
[   10.319029] mmc1: frequency set to 0 in disable function, this means the clock is already disabled.

<insert a card, mount>

[ 1569.970580] mmc0: new SDHC card at address d555
[ 1744.842376] mmcblk0: mmc0:d555 SD04G 3.79 GiB 
[ 1744.845224]  mmcblk0: p1
[ 1745.312968] EXT4-fs (mmcblk0p1): mounted filesystem without journal.

<umount, remove card>

[ 1795.000070] mmc0: card d555 removed
[ 1795.029868] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.037021] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.037043] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.037065] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.037086] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.058904] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.086940] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.093011] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.093048] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.093084] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.093115] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.116854] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.120881] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.120938] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.120972] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.121001] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.121030] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.121058] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
[ 1795.121083] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.

Questions:

* Are we expecting mmc_host_clk_gate_delayed() to be called so often
  with host->ios.clock == 0?  There's a full log with CONFIG_MMC_DEBUG=y
  at http://chris.printf.net/mmc-clkgate-debug in case it helps you see
  what's going on.

* If you think the current behavior should stay, we would at least want
  to convert it to a dev_dbg so that it's only visible under MMC_DEBUG=y,
  right?

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

2010/11/23 Chris Ball <cjb@laptop.org>:

> [   10.136067] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
> [   10.164089] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
> [   10.192076] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
> [   10.291095] mmc1: frequency set to 0 in disable function, this means the clock is already disabled.
> [   10.319029] mmc1: frequency set to 0 in disable function, this means the clock is already disabled.
>
> * Are we expecting mmc_host_clk_gate_delayed() to be called so often
>  with host->ios.clock == 0?

It didn't use to be like that back when I first wrote the patch, but
it seems to be like
that nowadays. So yes.

Did the gating work by the way? No strangeness because of it?

>  There's a full log with CONFIG_MMC_DEBUG=y
>  at http://chris.printf.net/mmc-clkgate-debug in case it helps you see
>  what's going on.
>
> * If you think the current behavior should stay, we would at least want
>  to convert it to a dev_dbg so that it's only visible under MMC_DEBUG=y,
>  right?

Hm?

It's pr_debug() already in host.c:
pr_debug("%s: frequency set to 0 in disable function, "
	 "this means the clock is already disabled.\n",
	 mmc_hostname(host));

pr_debug is only printed when -DDEBUG is set, which is what
MMC_DEBUG=y does isn't it?

But I can patch it to use dev_dbg() I think, does the below patch
solve the thing you want?

Yours,
Linus Walleij

From 75428e5cc51f1775389d5e3ad953e3b9edd43984 Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@stericsson.com>
Date: Wed, 24 Nov 2010 14:08:14 +0100
Subject: [PATCH] mmc: switch clockgating message from pr_debug to dev_dbg

This prints clock gating information with the dev_* macro instead.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mmc/core/host.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 92e3370..69fa70a 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -68,9 +68,9 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 	unsigned long flags;

 	if (!freq) {
-		pr_debug("%s: frequency set to 0 in disable function, "
-			 "this means the clock is already disabled.\n",
-			 mmc_hostname(host));
+		dev_dbg(mmc_dev(host), "frequency set to 0 in disable "
+			"function, this means the clock is already "
+			"disabled\n");
 		return;
 	}
 	/*
@@ -101,7 +101,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 		/* This will set host->ios.clock to 0 */
 		mmc_gate_clock(host);
 		spin_lock_irqsave(&host->clk_lock, flags);
-		pr_debug("%s: gated MCI clock\n", mmc_hostname(host));
+		dev_dbg(mmc_dev(host), "gated MCI clock\n");
 	}
 	spin_unlock_irqrestore(&host->clk_lock, flags);
 	mutex_unlock(&host->clk_gate_mutex);
@@ -136,7 +136,7 @@ void mmc_host_clk_ungate(struct mmc_host *host)
 		spin_unlock_irqrestore(&host->clk_lock, flags);
 		mmc_ungate_clock(host);
 		spin_lock_irqsave(&host->clk_lock, flags);
-		pr_debug("%s: ungated MCI clock\n", mmc_hostname(host));
+		dev_dbg(mmc_dev(host), "ungated MCI clock\n");
 	}
 	host->clk_requests++;
 	spin_unlock_irqrestore(&host->clk_lock, flags);
-- 
1.7.3.2

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

On Wed, Nov 24, 2010 at 02:15:57PM +0100, Linus Walleij wrote:
> > [   10.136067] mmc0: frequency set to 0 in disable function, this means the clock is already disabled.
> 
> It didn't use to be like that back when I first wrote the patch, but
> it seems to be like
> that nowadays. So yes.

Hm, okay.  Printing the same message seven times in a second is a little
undesirable, but we could leave it in until we're more confident that
the code's all working well.  It might help if the message made it
clearer that nothing is wrong, and this is an expected event.

> Did the gating work by the way? No strangeness because of it?

Yep, no strangeness so far.

> It's pr_debug() already in host.c:
> pr_debug("%s: frequency set to 0 in disable function, "
> 	 "this means the clock is already disabled.\n",
> 	 mmc_hostname(host));
> 
> pr_debug is only printed when -DDEBUG is set, which is what
> MMC_DEBUG=y does isn't it?

Ah, apologies, I understand now.  My linux-next build was too old for
"mmc: protect against clock-gating races", which does s/pr_err/pr_debug/
on that printk, so it was coming out unconditional on MMC_DEBUG.  I've
moved up to today's linux-next now.

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

Hi Linus,

On Wed, Nov 03, 2010 at 10:22:50AM +0100, Linus Walleij wrote:
> +/*
> + *	mmc_host_clk_exit - shut down clock gating code
> + *	@host: host with potential clock to control
> + */
> +static inline void mmc_host_clk_exit(struct mmc_host *host)
> +{
> +	/*
> +	 * Wait for any outstanding gate and then make sure we're
> +	 * ungated before exiting.
> +	 */
> +	if (cancel_work_sync(&host->clk_disable_work))
> +		mmc_host_clk_gate_delayed(host);
> +	if (host->clk_gated)
> +		mmc_host_clk_ungate(host);
> +	BUG_ON(host->clk_requests > 0);
> +}

I just hit the BUG_ON() above, when doing "rmmod sdhci-pci" on my x86
laptop running today's linux-next.  There was *no* SD card inserted,
and hadn't been one inserted all boot.

Let me know if you want me to try adding any extra debugging/patches.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

2010/12/21 Chris Ball <cjb@laptop.org>:

> Hi Linus,
>
> On Wed, Nov 03, 2010 at 10:22:50AM +0100, Linus Walleij wrote:
>> +/*
>> + *   mmc_host_clk_exit - shut down clock gating code
>> + *   @host: host with potential clock to control
>> + */
>> +static inline void mmc_host_clk_exit(struct mmc_host *host)
>> +{
>> +     /*
>> +      * Wait for any outstanding gate and then make sure we're
>> +      * ungated before exiting.
>> +      */
>> +     if (cancel_work_sync(&host->clk_disable_work))
>> +             mmc_host_clk_gate_delayed(host);
>> +     if (host->clk_gated)
>> +             mmc_host_clk_ungate(host);
>> +     BUG_ON(host->clk_requests > 0);
>> +}
>
> I just hit the BUG_ON() above, when doing "rmmod sdhci-pci" on my x86
> laptop running today's linux-next.  There was *no* SD card inserted,
> and hadn't been one inserted all boot.

Hm, it's a plain bug...

We make sure it's ungated the line above so clk_requests is always
== 1.

It should be BUG_ON(host->clk_requests > 1)

Can you try the below:

From: Linus Walleij <linus.walleij@stericsson.com>
Date: Wed, 22 Dec 2010 09:10:04 +0100
Subject: [PATCH] mmc: check for > 1 clk_requests

Since we make sure the clock is enabled in the mmc_host_clk_exit()
function we should expect a reference counter of 1, not 0.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mmc/core/host.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 92e3370..072d29c 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -235,7 +235,7 @@ static inline void mmc_host_clk_exit(struct mmc_host *host)
 		mmc_host_clk_gate_delayed(host);
 	if (host->clk_gated)
 		mmc_host_clk_ungate(host);
-	BUG_ON(host->clk_requests > 0);
+	BUG_ON(host->clk_requests > 1);
 }

 #else
-- 
1.7.3.3

Yours,
Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[David Vrabel]

On 22/12/2010 08:12, Linus Walleij wrote:
> 2010/12/21 Chris Ball <cjb@laptop.org>:
> 
>> Hi Linus,
>>
>> On Wed, Nov 03, 2010 at 10:22:50AM +0100, Linus Walleij wrote:
>>> +/*
>>> + *   mmc_host_clk_exit - shut down clock gating code
>>> + *   @host: host with potential clock to control
>>> + */
>>> +static inline void mmc_host_clk_exit(struct mmc_host *host)
>>> +{
>>> +     /*
>>> +      * Wait for any outstanding gate and then make sure we're
>>> +      * ungated before exiting.
>>> +      */
>>> +     if (cancel_work_sync(&host->clk_disable_work))
>>> +             mmc_host_clk_gate_delayed(host);
>>> +     if (host->clk_gated)
>>> +             mmc_host_clk_ungate(host);
>>> +     BUG_ON(host->clk_requests > 0);
>>> +}
>>
>> I just hit the BUG_ON() above, when doing "rmmod sdhci-pci" on my x86
>> laptop running today's linux-next.  There was *no* SD card inserted,
>> and hadn't been one inserted all boot.
> 
> Hm, it's a plain bug...
> 
> We make sure it's ungated the line above so clk_requests is always
> == 1.
> 
> It should be BUG_ON(host->clk_requests > 1)

Change to a WARN_ON() perhaps?

David

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Linus Walleij]

2010/12/22 David Vrabel <david.vrabel@csr.com>:

> [Me]
>> It should be BUG_ON(host->clk_requests > 1)
>
> Change to a WARN_ON() perhaps?

Sure! Chris do you prefer this one instead?

From: Linus Walleij <linus.walleij@stericsson.com>
Date: Wed, 22 Dec 2010 09:49:04 +0100
Subject: [PATCH] mmc: check for > 1 clk_requests

Since we make sure the clock is enabled in the mmc_host_clk_exit()
function we should expect a reference counter of 1, not 0.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mmc/core/host.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 92e3370..b3ac6c5 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -235,7 +235,8 @@ static inline void mmc_host_clk_exit(struct mmc_host *host)
 		mmc_host_clk_gate_delayed(host);
 	if (host->clk_gated)
 		mmc_host_clk_ungate(host);
-	BUG_ON(host->clk_requests > 0);
+	/* There should be only one user now */
+	WARN_ON(host->clk_requests > 1);
 }

 #else
-- 
1.7.3.3

Yours,
Linus Walleij

Re: [PATCH 1/2] mmc: agressive clocking framework v8

[Chris Ball]

Hi Linus,

On Wed, Dec 22, 2010 at 09:50:12AM +0100, Linus Walleij wrote:
> Sure! Chris do you prefer this one instead?
> 
> From: Linus Walleij <linus.walleij@stericsson.com>
> Date: Wed, 22 Dec 2010 09:49:04 +0100
> Subject: [PATCH] mmc: check for > 1 clk_requests
> 
> Since we make sure the clock is enabled in the mmc_host_clk_exit()
> function we should expect a reference counter of 1, not 0.
> 
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
>  drivers/mmc/core/host.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 92e3370..b3ac6c5 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -235,7 +235,8 @@ static inline void mmc_host_clk_exit(struct mmc_host *host)
>  		mmc_host_clk_gate_delayed(host);
>  	if (host->clk_gated)
>  		mmc_host_clk_ungate(host);
> -	BUG_ON(host->clk_requests > 0);
> +	/* There should be only one user now */
> +	WARN_ON(host->clk_requests > 1);
>  }
> 
>  #else

Thanks, both of you, have merged this to mmc-next now.  We'll do some
rebasing to fold it in later.

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child