[PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Ulf Hansson]

This patchset is remove all code related to MMC_CLKGATE. A significant
portion of code can then be removed from the core layer which would
simplify maintainance.

At the moment it seems like only some MIPS platforms were using
MMC_CLKGATE, but at the same time the corresponding host drivers
were not taking advantage of the mechanism. This made me feel confident
in removing the feature entirely from the mmc subsystem could be done.

Also do note that for host drivers that would like to implement clock
gating as a power save operation, using runtime PM is a far easier
way to address this. Several host drivers is already fully supporting
this as well.

Ulf Hansson (2):
  MIPS: db1235: Don't use MMC_CLKGATE
  mmc: core: Remove MMC_CLKGATE

 Documentation/mmc/mmc-dev-attrs.txt |   10 --
 arch/mips/configs/db1235_defconfig  |    1 -
 drivers/mmc/core/Kconfig            |   10 --
 drivers/mmc/core/core.c             |  116 +----------------
 drivers/mmc/core/core.h             |    3 -
 drivers/mmc/core/debugfs.c          |    5 -
 drivers/mmc/core/host.c             |  245 -----------------------------------
 drivers/mmc/core/mmc.c              |    6 +-
 drivers/mmc/core/sd.c               |   12 +-
 drivers/mmc/core/sdio.c             |    5 +-
 drivers/mmc/core/sdio_irq.c         |   10 +-
 include/linux/mmc/host.h            |   27 ----
 12 files changed, 12 insertions(+), 438 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] MIPS: db1235: Don't use MMC_CLKGATE

[Ulf Hansson]

As a first step in removing code for MMC_CLKGATE, MIPS db1235 defconfig
which is the only current user, shall move away from this option.

The mmc host drivers au1xmmc and jz4740_mmc, which are used on MIPS
don't support clock gating through MMC_CLKGATE, thus removing the
config option will have no effect on power save - clock gating wise.

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/mips/configs/db1235_defconfig |    1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/configs/db1235_defconfig b/arch/mips/configs/db1235_defconfig
index e2b4ad5..28e49f2 100644
--- a/arch/mips/configs/db1235_defconfig
+++ b/arch/mips/configs/db1235_defconfig
@@ -351,7 +351,6 @@ CONFIG_USB_OHCI_HCD=y
 CONFIG_USB_OHCI_HCD_PLATFORM=y
 CONFIG_USB_STORAGE=y
 CONFIG_MMC=y
-CONFIG_MMC_CLKGATE=y
 CONFIG_MMC_AU1X=y
 CONFIG_NEW_LEDS=y
 CONFIG_LEDS_CLASS=y
-- 
1.7.9.5

Re: [PATCH 1/2] MIPS: db1235: Don't use MMC_CLKGATE

[Ralf Baechle]

On Mon, Sep 30, 2013 at 04:41:59PM +0200, Ulf Hansson wrote:

> As a first step in removing code for MMC_CLKGATE, MIPS db1235 defconfig
> which is the only current user, shall move away from this option.
> 
> The mmc host drivers au1xmmc and jz4740_mmc, which are used on MIPS
> don't support clock gating through MMC_CLKGATE, thus removing the
> config option will have no effect on power save - clock gating wise.
> 
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Ralf Baechle <ralf@linux-mips.org>

Feel free to feed this MIPS patch to Linus via the MMC tree.

  Ralf

Re: [PATCH 1/2] MIPS: db1235: Don't use MMC_CLKGATE

[Ulf Hansson]

On 2 October 2013 11:24, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Mon, Sep 30, 2013 at 04:41:59PM +0200, Ulf Hansson wrote:
>
>> As a first step in removing code for MMC_CLKGATE, MIPS db1235 defconfig
>> which is the only current user, shall move away from this option.
>>
>> The mmc host drivers au1xmmc and jz4740_mmc, which are used on MIPS
>> don't support clock gating through MMC_CLKGATE, thus removing the
>> config option will have no effect on power save - clock gating wise.
>>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: linux-mips@linux-mips.org
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Acked-by: Ralf Baechle <ralf@linux-mips.org>
>
> Feel free to feed this MIPS patch to Linus via the MMC tree.
>
>   Ralf

Thanks Ralf!

Chris, do you mind taking this through your tree?

Kind regards
Ulf Hansson

[PATCH 2/2] mmc: core: Remove MMC_CLKGATE

[Ulf Hansson]

No platforms seems to be using MMC_CLKGATE anymore. Moreover the same
clock gating operations are easier to implement using runtime PM, which
several host drivers already had converted to.

By removing all related code for MMC_CLKGATE, a significant less amount
of code needs to be maintained.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/mmc/mmc-dev-attrs.txt |   10 --
 drivers/mmc/core/Kconfig            |   10 --
 drivers/mmc/core/core.c             |  116 +----------------
 drivers/mmc/core/core.h             |    3 -
 drivers/mmc/core/debugfs.c          |    5 -
 drivers/mmc/core/host.c             |  245 -----------------------------------
 drivers/mmc/core/mmc.c              |    6 +-
 drivers/mmc/core/sd.c               |   12 +-
 drivers/mmc/core/sdio.c             |    5 +-
 drivers/mmc/core/sdio_irq.c         |   10 +-
 include/linux/mmc/host.h            |   27 ----
 11 files changed, 12 insertions(+), 437 deletions(-)

diff --git a/Documentation/mmc/mmc-dev-attrs.txt b/Documentation/mmc/mmc-dev-attrs.txt
index 189bab0..caa5557 100644
--- a/Documentation/mmc/mmc-dev-attrs.txt
+++ b/Documentation/mmc/mmc-dev-attrs.txt
@@ -72,13 +72,3 @@ Note on raw_rpmb_size_mult:
 	"raw_rpmb_size_mult" is a mutliple of 128kB block.
 	RPMB size in byte is calculated by using the following equation:
 	RPMB partition size = 128kB x raw_rpmb_size_mult
-
-SD/MMC/SDIO Clock Gating Attribute
-==================================
-
-Read and write access is provided to following attribute.
-This attribute appears only if CONFIG_MMC_CLKGATE is enabled.
-
-	clkgate_delay	Tune the clock gating delay with desired value in milliseconds.
-
-echo <desired delay> > /sys/class/mmc_host/mmcX/clkgate_delay
diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index 269d072..bb22ffd 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -16,13 +16,3 @@ 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"
-	help
-	  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.
-
-	  If unsure, say N.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 006ead2..a5ce361 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -185,8 +185,6 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 
 		if (mrq->done)
 			mrq->done(mrq);
-
-		mmc_host_clk_release(host);
 	}
 }
 
@@ -251,7 +249,6 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 			mrq->stop->mrq = mrq;
 		}
 	}
-	mmc_host_clk_hold(host);
 	led_trigger_event(host->led, LED_FULL);
 	host->ops->request(host, mrq);
 }
@@ -482,11 +479,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 static void mmc_pre_req(struct mmc_host *host, struct mmc_request *mrq,
 		 bool is_first_req)
 {
-	if (host->ops->pre_req) {
-		mmc_host_clk_hold(host);
+	if (host->ops->pre_req)
 		host->ops->pre_req(host, mrq, is_first_req);
-		mmc_host_clk_release(host);
-	}
 }
 
 /**
@@ -501,11 +495,8 @@ static void mmc_pre_req(struct mmc_host *host, struct mmc_request *mrq,
 static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
 			 int err)
 {
-	if (host->ops->post_req) {
-		mmc_host_clk_hold(host);
+	if (host->ops->post_req)
 		host->ops->post_req(host, mrq, err);
-		mmc_host_clk_release(host);
-	}
 }
 
 /**
@@ -983,8 +974,6 @@ static inline void mmc_set_ios(struct mmc_host *host)
 		 ios->power_mode, ios->chip_select, ios->vdd,
 		 ios->bus_width, ios->timing);
 
-	if (ios->clock > 0)
-		mmc_set_ungated(host);
 	host->ops->set_ios(host, ios);
 }
 
@@ -993,17 +982,15 @@ static inline void mmc_set_ios(struct mmc_host *host)
  */
 void mmc_set_chip_select(struct mmc_host *host, int mode)
 {
-	mmc_host_clk_hold(host);
 	host->ios.chip_select = mode;
 	mmc_set_ios(host);
-	mmc_host_clk_release(host);
 }
 
 /*
  * Sets the host clock to the highest possible frequency that
  * is below "hz".
  */
-static void __mmc_set_clock(struct mmc_host *host, unsigned int hz)
+void mmc_set_clock(struct mmc_host *host, unsigned int hz)
 {
 	WARN_ON(hz < host->f_min);
 
@@ -1014,77 +1001,13 @@ static void __mmc_set_clock(struct mmc_host *host, unsigned int hz)
 	mmc_set_ios(host);
 }
 
-void mmc_set_clock(struct mmc_host *host, unsigned int hz)
-{
-	mmc_host_clk_hold(host);
-	__mmc_set_clock(host, hz);
-	mmc_host_clk_release(host);
-}
-
-#ifdef CONFIG_MMC_CLKGATE
-/*
- * This gates the clock by setting it to 0 Hz.
- */
-void mmc_gate_clock(struct mmc_host *host)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&host->clk_lock, flags);
-	host->clk_old = host->ios.clock;
-	host->ios.clock = 0;
-	host->clk_gated = true;
-	spin_unlock_irqrestore(&host->clk_lock, flags);
-	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 initialization,
-	 * 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);
-		/* This call will also set host->clk_gated to false */
-		__mmc_set_clock(host, host->clk_old);
-	}
-}
-
-void mmc_set_ungated(struct mmc_host *host)
-{
-	unsigned long flags;
-
-	/*
-	 * We've been given a new frequency while the clock is gated,
-	 * so make sure we regard this as ungating it.
-	 */
-	spin_lock_irqsave(&host->clk_lock, flags);
-	host->clk_gated = false;
-	spin_unlock_irqrestore(&host->clk_lock, flags);
-}
-
-#else
-void mmc_set_ungated(struct mmc_host *host)
-{
-}
-#endif
-
 /*
  * Change the bus mode (open drain/push-pull) of a host.
  */
 void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode)
 {
-	mmc_host_clk_hold(host);
 	host->ios.bus_mode = mode;
 	mmc_set_ios(host);
-	mmc_host_clk_release(host);
 }
 
 /*
@@ -1092,10 +1015,8 @@ void mmc_set_bus_mode(struct mmc_host *host, unsigned int mode)
  */
 void mmc_set_bus_width(struct mmc_host *host, unsigned int width)
 {
-	mmc_host_clk_hold(host);
 	host->ios.bus_width = width;
 	mmc_set_ios(host);
-	mmc_host_clk_release(host);
 }
 
 /**
@@ -1365,10 +1286,8 @@ u32 mmc_select_voltage(struct mmc_host *host, u32 ocr)
 
 		ocr &= 3 << bit;
 
-		mmc_host_clk_hold(host);
 		host->ios.vdd = bit;
 		mmc_set_ios(host);
-		mmc_host_clk_release(host);
 	} else {
 		pr_warning("%s: host doesn't support card's voltages\n",
 				mmc_hostname(host));
@@ -1384,11 +1303,8 @@ int __mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
 	int old_signal_voltage = host->ios.signal_voltage;
 
 	host->ios.signal_voltage = signal_voltage;
-	if (host->ops->start_signal_voltage_switch) {
-		mmc_host_clk_hold(host);
+	if (host->ops->start_signal_voltage_switch)
 		err = host->ops->start_signal_voltage_switch(host, &host->ios);
-		mmc_host_clk_release(host);
-	}
 
 	if (err)
 		host->ios.signal_voltage = old_signal_voltage;
@@ -1433,7 +1349,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage)
 	if (!mmc_host_is_spi(host) && (cmd.resp[0] & R1_ERROR))
 		return -EIO;
 
-	mmc_host_clk_hold(host);
 	/*
 	 * The card should drive cmd and dat[0:3] low immediately
 	 * after the response of cmd11, but wait 1 ms to be sure
@@ -1482,8 +1397,6 @@ power_cycle:
 		mmc_power_cycle(host);
 	}
 
-	mmc_host_clk_release(host);
-
 	return err;
 }
 
@@ -1492,10 +1405,8 @@ power_cycle:
  */
 void mmc_set_timing(struct mmc_host *host, unsigned int timing)
 {
-	mmc_host_clk_hold(host);
 	host->ios.timing = timing;
 	mmc_set_ios(host);
-	mmc_host_clk_release(host);
 }
 
 /*
@@ -1503,10 +1414,8 @@ void mmc_set_timing(struct mmc_host *host, unsigned int timing)
  */
 void mmc_set_driver_type(struct mmc_host *host, unsigned int drv_type)
 {
-	mmc_host_clk_hold(host);
 	host->ios.drv_type = drv_type;
 	mmc_set_ios(host);
-	mmc_host_clk_release(host);
 }
 
 /*
@@ -1527,8 +1436,6 @@ void mmc_power_up(struct mmc_host *host)
 	if (host->ios.power_mode == MMC_POWER_ON)
 		return;
 
-	mmc_host_clk_hold(host);
-
 	/* If ocr is set, we use it */
 	if (host->ocr)
 		bit = ffs(host->ocr) - 1;
@@ -1565,8 +1472,6 @@ void mmc_power_up(struct mmc_host *host)
 	 * time required to reach a stable voltage.
 	 */
 	mmc_delay(10);
-
-	mmc_host_clk_release(host);
 }
 
 void mmc_power_off(struct mmc_host *host)
@@ -1574,8 +1479,6 @@ void mmc_power_off(struct mmc_host *host)
 	if (host->ios.power_mode == MMC_POWER_OFF)
 		return;
 
-	mmc_host_clk_hold(host);
-
 	host->ios.clock = 0;
 	host->ios.vdd = 0;
 
@@ -1601,8 +1504,6 @@ void mmc_power_off(struct mmc_host *host)
 	 * can be successfully turned on again.
 	 */
 	mmc_delay(1);
-
-	mmc_host_clk_release(host);
 }
 
 void mmc_power_cycle(struct mmc_host *host)
@@ -2217,9 +2118,7 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
 {
 	if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
 		return;
-	mmc_host_clk_hold(host);
 	host->ops->hw_reset(host);
-	mmc_host_clk_release(host);
 }
 
 int mmc_can_reset(struct mmc_card *card)
@@ -2251,7 +2150,6 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 	if (!mmc_can_reset(card))
 		return -EOPNOTSUPP;
 
-	mmc_host_clk_hold(host);
 	mmc_set_clock(host, host->f_init);
 
 	host->ops->hw_reset(host);
@@ -2266,10 +2164,8 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 			cmd.arg = card->rca << 16;
 		cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
 		err = mmc_wait_for_cmd(card->host, &cmd, 0);
-		if (!err) {
-			mmc_host_clk_release(host);
+		if (!err)
 			return -ENOSYS;
-		}
 	}
 
 	host->card->state &= ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_DDR);
@@ -2284,8 +2180,6 @@ static int mmc_do_hw_reset(struct mmc_host *host, int check)
 	host->ios.timing = MMC_TIMING_LEGACY;
 	mmc_set_ios(host);
 
-	mmc_host_clk_release(host);
-
 	return host->bus_ops->power_restore(host);
 }
 
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 5345d15..f7239ec 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -36,9 +36,6 @@ 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_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);
 u32 mmc_select_voltage(struct mmc_host *host, u32 ocr);
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 54829c0..5509fef 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -224,11 +224,6 @@ 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
 #ifdef CONFIG_FAIL_MMC_REQUEST
 	if (fail_request)
 		setup_fault_attr(&fail_default_attr, fail_request);
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 49bc403..fa6a903 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -57,246 +57,6 @@ void mmc_unregister_host_class(void)
 static DEFINE_IDR(mmc_host_idr);
 static DEFINE_SPINLOCK(mmc_host_lock);
 
-#ifdef CONFIG_MMC_CLKGATE
-static ssize_t clkgate_delay_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct mmc_host *host = cls_dev_to_mmc_host(dev);
-	return snprintf(buf, PAGE_SIZE, "%lu\n", host->clkgate_delay);
-}
-
-static ssize_t clkgate_delay_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct mmc_host *host = cls_dev_to_mmc_host(dev);
-	unsigned long flags, value;
-
-	if (kstrtoul(buf, 0, &value))
-		return -EINVAL;
-
-	spin_lock_irqsave(&host->clk_lock, flags);
-	host->clkgate_delay = value;
-	spin_unlock_irqrestore(&host->clk_lock, flags);
-	return count;
-}
-
-/*
- * 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;
-
-	if (!freq) {
-		pr_debug("%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);
-
-	/*
-	 * 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 (!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 */
-		spin_unlock_irqrestore(&host->clk_lock, flags);
-		return;
-	}
-	mutex_lock(&host->clk_gate_mutex);
-	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));
-	}
-	spin_unlock_irqrestore(&host->clk_lock, flags);
-	mutex_unlock(&host->clk_gate_mutex);
-}
-
-/*
- * 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_gate_work.work);
-
-	mmc_host_clk_gate_delayed(host);
-}
-
-/**
- *	mmc_host_clk_hold - ungate hardware MCI clocks
- *	@host: host to ungate.
- *
- *	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_hold(struct mmc_host *host)
-{
-	unsigned long flags;
-
-	/* cancel any clock gating work scheduled by mmc_host_clk_release() */
-	cancel_delayed_work_sync(&host->clk_gate_work);
-	mutex_lock(&host->clk_gate_mutex);
-	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);
-	mutex_unlock(&host->clk_gate_mutex);
-}
-
-/**
- *	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.
-	 */
-	return !(card->quirks & MMC_QUIRK_BROKEN_CLK_GATING);
-}
-
-/**
- *	mmc_host_clk_release - gate off hardware MCI clocks
- *	@host: host to gate.
- *
- *	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_release(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)
-		schedule_delayed_work(&host->clk_gate_work,
-				      msecs_to_jiffies(host->clkgate_delay));
-	spin_unlock_irqrestore(&host->clk_lock, flags);
-}
-
-/**
- *	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)
-{
-	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;
-	/* Hold MCI clock for 8 cycles by default */
-	host->clk_delay = 8;
-	/*
-	 * Default clock gating delay is 0ms to avoid wasting power.
-	 * This value can be tuned by writing into sysfs entry.
-	 */
-	host->clkgate_delay = 0;
-	host->clk_gated = false;
-	INIT_DELAYED_WORK(&host->clk_gate_work, mmc_host_clk_gate_work);
-	spin_lock_init(&host->clk_lock);
-	mutex_init(&host->clk_gate_mutex);
-}
-
-/**
- *	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_delayed_work_sync(&host->clk_gate_work))
-		mmc_host_clk_gate_delayed(host);
-	if (host->clk_gated)
-		mmc_host_clk_hold(host);
-	/* There should be only one user now */
-	WARN_ON(host->clk_requests > 1);
-}
-
-static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
-{
-	host->clkgate_delay_attr.show = clkgate_delay_show;
-	host->clkgate_delay_attr.store = clkgate_delay_store;
-	sysfs_attr_init(&host->clkgate_delay_attr.attr);
-	host->clkgate_delay_attr.attr.name = "clkgate_delay";
-	host->clkgate_delay_attr.attr.mode = S_IRUGO | S_IWUSR;
-	if (device_create_file(&host->class_dev, &host->clkgate_delay_attr))
-		pr_err("%s: Failed to create clkgate_delay sysfs entry\n",
-				mmc_hostname(host));
-}
-#else
-
-static inline void mmc_host_clk_init(struct mmc_host *host)
-{
-}
-
-static inline void mmc_host_clk_exit(struct mmc_host *host)
-{
-}
-
-static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
-{
-}
-
-#endif
-
 /**
  *	mmc_of_parse() - parse host's device-tree node
  *	@host: host whose node should be parsed.
@@ -474,8 +234,6 @@ 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);
-
 	mutex_init(&host->slot.lock);
 	host->slot.cd_irq = -EINVAL;
 
@@ -530,7 +288,6 @@ int mmc_add_host(struct mmc_host *host)
 #ifdef CONFIG_DEBUG_FS
 	mmc_add_host_debugfs(host);
 #endif
-	mmc_host_clk_sysfs_init(host);
 
 	mmc_start_host(host);
 	register_pm_notifier(&host->pm_notify);
@@ -560,8 +317,6 @@ 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);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 6d02012..221c903 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1147,12 +1147,10 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 		 * 4. execute tuning for HS200
 		 */
 		if ((host->caps2 & MMC_CAP2_HS200) &&
-		    card->host->ops->execute_tuning) {
-			mmc_host_clk_hold(card->host);
+		    card->host->ops->execute_tuning)
 			err = card->host->ops->execute_tuning(card->host,
 				MMC_SEND_TUNING_BLOCK_HS200);
-			mmc_host_clk_release(card->host);
-		}
+
 		if (err) {
 			pr_warning("%s: tuning execution failed\n",
 				   mmc_hostname(card->host));
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 5e8823d..c52a701 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -420,11 +420,9 @@ static int sd_select_driver_type(struct mmc_card *card, u8 *status)
 	 * information and let the hardware specific code
 	 * return what is possible given the options
 	 */
-	mmc_host_clk_hold(card->host);
 	drive_strength = card->host->ops->select_drive_strength(
 		card->sw_caps.uhs_max_dtr,
 		host_drv_type, card_drv_type);
-	mmc_host_clk_release(card->host);
 
 	err = mmc_sd_switch(card, 1, 2, drive_strength, status);
 	if (err)
@@ -655,12 +653,9 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
 	 */
 	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
 			(card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-			 card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
-		mmc_host_clk_hold(card->host);
+			 card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
 		err = card->host->ops->execute_tuning(card->host,
 						      MMC_SEND_TUNING_BLOCK);
-		mmc_host_clk_release(card->host);
-	}
 
 out:
 	kfree(status);
@@ -862,11 +857,8 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
 	if (!reinit) {
 		int ro = -1;
 
-		if (host->ops->get_ro) {
-			mmc_host_clk_hold(card->host);
+		if (host->ops->get_ro)
 			ro = host->ops->get_ro(host);
-			mmc_host_clk_release(card->host);
-		}
 
 		if (ro < 0) {
 			pr_warning("%s: host does not "
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 80d89cff..3b884a1 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -569,12 +569,9 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
 	 */
 	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
 			((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
-			 (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))) {
-		mmc_host_clk_hold(card->host);
+			 (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)))
 		err = card->host->ops->execute_tuning(card->host,
 						      MMC_SEND_TUNING_BLOCK);
-		mmc_host_clk_release(card->host);
-	}
 
 out:
 
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 3d8ceb4..9dd0462 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -149,21 +149,15 @@ static int sdio_irq_thread(void *_host)
 		}
 
 		set_current_state(TASK_INTERRUPTIBLE);
-		if (host->caps & MMC_CAP_SDIO_IRQ) {
-			mmc_host_clk_hold(host);
+		if (host->caps & MMC_CAP_SDIO_IRQ)
 			host->ops->enable_sdio_irq(host, 1);
-			mmc_host_clk_release(host);
-		}
 		if (!kthread_should_stop())
 			schedule_timeout(period);
 		set_current_state(TASK_RUNNING);
 	} while (!kthread_should_stop());
 
-	if (host->caps & MMC_CAP_SDIO_IRQ) {
-		mmc_host_clk_hold(host);
+	if (host->caps & MMC_CAP_SDIO_IRQ)
 		host->ops->enable_sdio_irq(host, 0);
-		mmc_host_clk_release(host);
-	}
 
 	pr_debug("%s: IRQ thread exiting with code %d\n",
 		 mmc_hostname(host), ret);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 3b0c33a..7d61cba 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -284,18 +284,6 @@ 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 */
-	struct delayed_work	clk_gate_work; /* delayed clock gate */
-	unsigned int		clk_old;	/* old clock value cache */
-	spinlock_t		clk_lock;	/* lock for clk fields */
-	struct mutex		clk_gate_mutex;	/* mutex for clock gating */
-	struct device_attribute clkgate_delay_attr;
-	unsigned long           clkgate_delay;
-#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 */
@@ -468,23 +456,8 @@ static inline int mmc_host_packed_wr(struct mmc_host *host)
 	return host->caps2 & MMC_CAP2_PACKED_WR;
 }
 
-#ifdef CONFIG_MMC_CLKGATE
-void mmc_host_clk_hold(struct mmc_host *host);
-void mmc_host_clk_release(struct mmc_host *host);
-unsigned int mmc_host_clk_rate(struct mmc_host *host);
-
-#else
-static inline void mmc_host_clk_hold(struct mmc_host *host)
-{
-}
-
-static inline void mmc_host_clk_release(struct mmc_host *host)
-{
-}
-
 static inline unsigned int mmc_host_clk_rate(struct mmc_host *host)
 {
 	return host->ios.clock;
 }
-#endif
 #endif /* LINUX_MMC_HOST_H */
-- 
1.7.9.5

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Guennadi Liakhovetski]

Hi Ulf

On Mon, 30 Sep 2013, Ulf Hansson wrote:

> This patchset is remove all code related to MMC_CLKGATE. A significant
> portion of code can then be removed from the core layer which would
> simplify maintainance.
> 
> At the moment it seems like only some MIPS platforms were using
> MMC_CLKGATE, but at the same time the corresponding host drivers
> were not taking advantage of the mechanism. This made me feel confident
> in removing the feature entirely from the mmc subsystem could be done.
> 
> Also do note that for host drivers that would like to implement clock
> gating as a power save operation, using runtime PM is a far easier
> way to address this. Several host drivers is already fully supporting
> this as well.
> 
> Ulf Hansson (2):
>   MIPS: db1235: Don't use MMC_CLKGATE
>   mmc: core: Remove MMC_CLKGATE

Do I understand correctly, that you're proposing to remove the aggressive 
clock gating feature from MMC? If so, then please take into account my 
NAK. At least two SD / MMC drivers, that I'm aware of, actively used and 
developed on multiple platforms - sh_mmcif and tmio / sdhi implement MMC 
clock gating and rely on it to save power between IO.

Thanks
Guennadi

> 
>  Documentation/mmc/mmc-dev-attrs.txt |   10 --
>  arch/mips/configs/db1235_defconfig  |    1 -
>  drivers/mmc/core/Kconfig            |   10 --
>  drivers/mmc/core/core.c             |  116 +----------------
>  drivers/mmc/core/core.h             |    3 -
>  drivers/mmc/core/debugfs.c          |    5 -
>  drivers/mmc/core/host.c             |  245 -----------------------------------
>  drivers/mmc/core/mmc.c              |    6 +-
>  drivers/mmc/core/sd.c               |   12 +-
>  drivers/mmc/core/sdio.c             |    5 +-
>  drivers/mmc/core/sdio_irq.c         |   10 +-
>  include/linux/mmc/host.h            |   27 ----
>  12 files changed, 12 insertions(+), 438 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Ulf Hansson]

On 30 September 2013 23:10, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Ulf
>
> On Mon, 30 Sep 2013, Ulf Hansson wrote:
>
>> This patchset is remove all code related to MMC_CLKGATE. A significant
>> portion of code can then be removed from the core layer which would
>> simplify maintainance.
>>
>> At the moment it seems like only some MIPS platforms were using
>> MMC_CLKGATE, but at the same time the corresponding host drivers
>> were not taking advantage of the mechanism. This made me feel confident
>> in removing the feature entirely from the mmc subsystem could be done.
>>
>> Also do note that for host drivers that would like to implement clock
>> gating as a power save operation, using runtime PM is a far easier
>> way to address this. Several host drivers is already fully supporting
>> this as well.
>>
>> Ulf Hansson (2):
>>   MIPS: db1235: Don't use MMC_CLKGATE
>>   mmc: core: Remove MMC_CLKGATE
>
> Do I understand correctly, that you're proposing to remove the aggressive
> clock gating feature from MMC? If so, then please take into account my
> NAK. At least two SD / MMC drivers, that I'm aware of, actively used and
> developed on multiple platforms - sh_mmcif and tmio / sdhi implement MMC
> clock gating and rely on it to save power between IO.

Guennadi, thanks for your NAK, you have understood correctly what my
intention is. :-)

Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
tmio), I realize that they use runtime PM in an awkward way. I would
suggest to move parts of the clock gating from .set_ios to runtime PM
callbacks instead. Obviously proper calls to pm_runtime_get|put needs
to be adopted as well. pm_runtime_get|put should be used to make sure
your "runtime resources" are active while I/O operations are ongoing,
they aren't in those drivers as of today.

So to give you some more details about why I wanted to push for a
removal of MMC_CLKGATE. You have one reason above. I want host drivers
that considers power save at request inactivity, to implement proper
runtime PM support and to do clock gating as a part of runtime PM.
There are already good references, like mmci and sdhci-* for example.

MMC_CLKGATE is an old way of dealing with clock gating for power save,
runtime PM is better suited and has been there now for a long time
now. It is time to switch.

I don't think mmc clock gating should be depending on a separate
kernel config as MMC_CLKGATE, which likely is the reason to why there
are no defconfig that has this option enabled. Instead RUNTIME_PM
already has this implicit meaning of handling with power save at
request inactivity.

Finally, it is nice to remove code from a maintenance point of view.
The MMC_CLKGATE code is being used in a lot of places around the core
layer, since "mmc_host_clk_hold|release" needs to be called so many
times.

What are your thoughts on the way forward? Could we adopt fully
runtime pm support for sh_mmcif sh_mobile_sdhci soon?

Kind regards
Ulf Hansson

>
> Thanks
> Guennadi
>
>>
>>  Documentation/mmc/mmc-dev-attrs.txt |   10 --
>>  arch/mips/configs/db1235_defconfig  |    1 -
>>  drivers/mmc/core/Kconfig            |   10 --
>>  drivers/mmc/core/core.c             |  116 +----------------
>>  drivers/mmc/core/core.h             |    3 -
>>  drivers/mmc/core/debugfs.c          |    5 -
>>  drivers/mmc/core/host.c             |  245 -----------------------------------
>>  drivers/mmc/core/mmc.c              |    6 +-
>>  drivers/mmc/core/sd.c               |   12 +-
>>  drivers/mmc/core/sdio.c             |    5 +-
>>  drivers/mmc/core/sdio_irq.c         |   10 +-
>>  include/linux/mmc/host.h            |   27 ----
>>  12 files changed, 12 insertions(+), 438 deletions(-)
>>
>> --
>> 1.7.9.5
>>
>> --
>> 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
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Guennadi Liakhovetski]

Hi Ulf

On Tue, 1 Oct 2013, Ulf Hansson wrote:

> On 30 September 2013 23:10, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Ulf
> >
> > On Mon, 30 Sep 2013, Ulf Hansson wrote:
> >
> >> This patchset is remove all code related to MMC_CLKGATE. A significant
> >> portion of code can then be removed from the core layer which would
> >> simplify maintainance.
> >>
> >> At the moment it seems like only some MIPS platforms were using
> >> MMC_CLKGATE, but at the same time the corresponding host drivers
> >> were not taking advantage of the mechanism. This made me feel confident
> >> in removing the feature entirely from the mmc subsystem could be done.
> >>
> >> Also do note that for host drivers that would like to implement clock
> >> gating as a power save operation, using runtime PM is a far easier
> >> way to address this. Several host drivers is already fully supporting
> >> this as well.
> >>
> >> Ulf Hansson (2):
> >>   MIPS: db1235: Don't use MMC_CLKGATE
> >>   mmc: core: Remove MMC_CLKGATE
> >
> > Do I understand correctly, that you're proposing to remove the aggressive
> > clock gating feature from MMC? If so, then please take into account my
> > NAK. At least two SD / MMC drivers, that I'm aware of, actively used and
> > developed on multiple platforms - sh_mmcif and tmio / sdhi implement MMC
> > clock gating and rely on it to save power between IO.
> 
> Guennadi, thanks for your NAK, you have understood correctly what my
> intention is. :-)
> 
> Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
> tmio), I realize that they use runtime PM in an awkward way. I would
> suggest to move parts of the clock gating from .set_ios to runtime PM
> callbacks instead. Obviously proper calls to pm_runtime_get|put needs
> to be adopted as well. pm_runtime_get|put should be used to make sure
> your "runtime resources" are active while I/O operations are ongoing,
> they aren't in those drivers as of today.
> 
> So to give you some more details about why I wanted to push for a
> removal of MMC_CLKGATE. You have one reason above. I want host drivers
> that considers power save at request inactivity, to implement proper
> runtime PM support and to do clock gating as a part of runtime PM.
> There are already good references, like mmci and sdhci-* for example.
> 
> MMC_CLKGATE is an old way of dealing with clock gating for power save,
> runtime PM is better suited and has been there now for a long time
> now. It is time to switch.
> 
> I don't think mmc clock gating should be depending on a separate
> kernel config as MMC_CLKGATE, which likely is the reason to why there
> are no defconfig that has this option enabled. Instead RUNTIME_PM
> already has this implicit meaning of handling with power save at
> request inactivity.
> 
> Finally, it is nice to remove code from a maintenance point of view.
> The MMC_CLKGATE code is being used in a lot of places around the core
> layer, since "mmc_host_clk_hold|release" needs to be called so many
> times.
> 
> What are your thoughts on the way forward? Could we adopt fully
> runtime pm support for sh_mmcif sh_mobile_sdhci soon?

Sorry, I don't have any information whether or when this will be done.

Thanks
Guennadi

> Kind regards
> Ulf Hansson
> 
> >
> > Thanks
> > Guennadi
> >
> >>
> >>  Documentation/mmc/mmc-dev-attrs.txt |   10 --
> >>  arch/mips/configs/db1235_defconfig  |    1 -
> >>  drivers/mmc/core/Kconfig            |   10 --
> >>  drivers/mmc/core/core.c             |  116 +----------------
> >>  drivers/mmc/core/core.h             |    3 -
> >>  drivers/mmc/core/debugfs.c          |    5 -
> >>  drivers/mmc/core/host.c             |  245 -----------------------------------
> >>  drivers/mmc/core/mmc.c              |    6 +-
> >>  drivers/mmc/core/sd.c               |   12 +-
> >>  drivers/mmc/core/sdio.c             |    5 +-
> >>  drivers/mmc/core/sdio_irq.c         |   10 +-
> >>  include/linux/mmc/host.h            |   27 ----
> >>  12 files changed, 12 insertions(+), 438 deletions(-)
> >>
> >> --
> >> 1.7.9.5
> >>
> >> --
> >> 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
> >>
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Guennadi Liakhovetski]

Hi Ulf,

Let me try to add a bit more to your explanations below.

On Tue, 1 Oct 2013, Guennadi Liakhovetski wrote:

> Hi Ulf
> 
> On Tue, 1 Oct 2013, Ulf Hansson wrote:
> 
> > On 30 September 2013 23:10, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > Hi Ulf
> > >
> > > On Mon, 30 Sep 2013, Ulf Hansson wrote:
> > >
> > >> This patchset is remove all code related to MMC_CLKGATE. A significant
> > >> portion of code can then be removed from the core layer which would
> > >> simplify maintainance.
> > >>
> > >> At the moment it seems like only some MIPS platforms were using
> > >> MMC_CLKGATE, but at the same time the corresponding host drivers
> > >> were not taking advantage of the mechanism. This made me feel confident
> > >> in removing the feature entirely from the mmc subsystem could be done.
> > >>
> > >> Also do note that for host drivers that would like to implement clock
> > >> gating as a power save operation, using runtime PM is a far easier
> > >> way to address this. Several host drivers is already fully supporting
> > >> this as well.
> > >>
> > >> Ulf Hansson (2):
> > >>   MIPS: db1235: Don't use MMC_CLKGATE
> > >>   mmc: core: Remove MMC_CLKGATE
> > >
> > > Do I understand correctly, that you're proposing to remove the aggressive
> > > clock gating feature from MMC? If so, then please take into account my
> > > NAK. At least two SD / MMC drivers, that I'm aware of, actively used and
> > > developed on multiple platforms - sh_mmcif and tmio / sdhi implement MMC
> > > clock gating and rely on it to save power between IO.
> > 
> > Guennadi, thanks for your NAK, you have understood correctly what my
> > intention is. :-)
> > 
> > Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
> > tmio), I realize that they use runtime PM in an awkward way.

I'm not sure what exactly you find awkward there. I know that a lot of 
work has been invested in runtime PM and clock gating on those two 
drivers, and the results were rather good: we were able to runtime suspend 
and wake up controllers according to .power_mode as provided to drivers' 
.set_ios() methods. We were able to use power domains and show, that even 
when the whole domain is powered up and down as a result of our runtime 
PM, we still can communicate with MMC, SD, and SDIO cards in slots. We 
added support for regulators. We also added support for aggressive MMC 
clock gating, which allowed us to save power by gating the clock _when_ it 
was safe, as decided by higher level protocols.

E.g. look at this comment in tmio_mmc_pio.c:

/*
 * We differentiate between the following 3 power states:
 * 1. card slot powered off, controller stopped. This is used, when either there
 *    is no card in the slot, or the card really has to be powered down.
 * 2. card slot powered on, controller stopped. This is used, when a card is in
 *    the slot, but no activity is currently taking place. This is a power-
 *    saving mode with card-state preserved. This state can be entered, e.g.
 *    when MMC clock-gating is used.
 * 3. card slot powered on, controller running. This is the actual active state.
 */
enum tmio_mmc_power {
	TMIO_MMC_OFF_STOP,	/* card power off, controller stopped */
	TMIO_MMC_ON_STOP,	/* card power on, controller stopped */
	TMIO_MMC_ON_RUN,	/* card power on, controller running */
};

Now, if you remove clock gating, would you still be able to really support 
all these modes? IIUC, the clock gating is there for higher level MMC/SD 
protocols (MMC, SD, SDHC, SDXC, SDIO,...) to inform the mmc core and 
controller drivers, that according to that protocol the clock can be 
switched off now. It seems to me, that you're just completely removing 
that functionality. Neither the mmc core nor low level controller drivers 
can know when the clock can be switched off. So, I don't see how runtime 
PM by itself can solve the problem. You can choose a different approach to 
signal idle / power-saving states from protocol drivers, but you anyway 
need to actually do that. In your patch "mmc: core: Remove MMC_CLKGATE" 
however, you just remove those calls from sd.c and mmc.c, so, I don't see 
how that functionality can be implemented after your proposed patches.

Thanks
Guennadi

> >  I would
> > suggest to move parts of the clock gating from .set_ios to runtime PM
> > callbacks instead. Obviously proper calls to pm_runtime_get|put needs
> > to be adopted as well. pm_runtime_get|put should be used to make sure
> > your "runtime resources" are active while I/O operations are ongoing,
> > they aren't in those drivers as of today.
> > 
> > So to give you some more details about why I wanted to push for a
> > removal of MMC_CLKGATE. You have one reason above. I want host drivers
> > that considers power save at request inactivity, to implement proper
> > runtime PM support and to do clock gating as a part of runtime PM.
> > There are already good references, like mmci and sdhci-* for example.
> > 
> > MMC_CLKGATE is an old way of dealing with clock gating for power save,
> > runtime PM is better suited and has been there now for a long time
> > now. It is time to switch.
> > 
> > I don't think mmc clock gating should be depending on a separate
> > kernel config as MMC_CLKGATE, which likely is the reason to why there
> > are no defconfig that has this option enabled. Instead RUNTIME_PM
> > already has this implicit meaning of handling with power save at
> > request inactivity.
> > 
> > Finally, it is nice to remove code from a maintenance point of view.
> > The MMC_CLKGATE code is being used in a lot of places around the core
> > layer, since "mmc_host_clk_hold|release" needs to be called so many
> > times.
> > 
> > What are your thoughts on the way forward? Could we adopt fully
> > runtime pm support for sh_mmcif sh_mobile_sdhci soon?
> 
> Sorry, I don't have any information whether or when this will be done.
> 
> Thanks
> Guennadi
> 
> > Kind regards
> > Ulf Hansson
> > 
> > >
> > > Thanks
> > > Guennadi
> > >
> > >>
> > >>  Documentation/mmc/mmc-dev-attrs.txt |   10 --
> > >>  arch/mips/configs/db1235_defconfig  |    1 -
> > >>  drivers/mmc/core/Kconfig            |   10 --
> > >>  drivers/mmc/core/core.c             |  116 +----------------
> > >>  drivers/mmc/core/core.h             |    3 -
> > >>  drivers/mmc/core/debugfs.c          |    5 -
> > >>  drivers/mmc/core/host.c             |  245 -----------------------------------
> > >>  drivers/mmc/core/mmc.c              |    6 +-
> > >>  drivers/mmc/core/sd.c               |   12 +-
> > >>  drivers/mmc/core/sdio.c             |    5 +-
> > >>  drivers/mmc/core/sdio_irq.c         |   10 +-
> > >>  include/linux/mmc/host.h            |   27 ----
> > >>  12 files changed, 12 insertions(+), 438 deletions(-)
> > >>
> > >> --
> > >> 1.7.9.5
> > >>
> > >> --
> > >> 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
> > >>
> > >
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Ulf Hansson]

>> > Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
>> > tmio), I realize that they use runtime PM in an awkward way.
>
> I'm not sure what exactly you find awkward there. I know that a lot of
> work has been invested in runtime PM and clock gating on those two
> drivers, and the results were rather good: we were able to runtime suspend
> and wake up controllers according to .power_mode as provided to drivers'
> .set_ios() methods. We were able to use power domains and show, that even
> when the whole domain is powered up and down as a result of our runtime
> PM, we still can communicate with MMC, SD, and SDIO cards in slots. We
> added support for regulators. We also added support for aggressive MMC
> clock gating, which allowed us to save power by gating the clock _when_ it
> was safe, as decided by higher level protocols.

By comparing with other mmc host drivers you see that the runtime PM
implementation differs significantly for sh_mmcif and sh_mobile_sdhci.
I am not saying it is wrong, just question it to try to understand
why. And maybe we can improve something as well.

>
> E.g. look at this comment in tmio_mmc_pio.c:
>
> /*
>  * We differentiate between the following 3 power states:
>  * 1. card slot powered off, controller stopped. This is used, when either there
>  *    is no card in the slot, or the card really has to be powered down.

I guess the "card slot" be interpreted as the core power to the card, VCC/VDD?

>  * 2. card slot powered on, controller stopped. This is used, when a card is in
>  *    the slot, but no activity is currently taking place. This is a power-
>  *    saving mode with card-state preserved. This state can be entered, e.g.
>  *    when MMC clock-gating is used.
>  * 3. card slot powered on, controller running. This is the actual active state.
>  */

Some questions/statements, please shoot them down if possible. :-)

1.
Is the controller involved in providing the actual power to the card
(VCC/VDD)? Looking at the code in .set_ios for sh_mmcif, it seems like
the external regulator (mmc->supply.vmmc) is the one that handles this
on it's own. Especially since power state 3) exists as well.

If my assumption is correct, my best guess is that we shall be able to
rework the runtime PM support as my patchset for sh_mmcif tried to do.
Unless I missed something and there are some other obstacles to
consider from a power domain point of view.

2.
Regarding clock handling. Are there any specific reason to why you
prevent the power domain (though runtime PM) from being dropped while
the clock is enabled (through clk_enable)? That also seems like
unnecessary.

> enum tmio_mmc_power {
>         TMIO_MMC_OFF_STOP,      /* card power off, controller stopped */
>         TMIO_MMC_ON_STOP,       /* card power on, controller stopped */
>         TMIO_MMC_ON_RUN,        /* card power on, controller running */
> };
>
> Now, if you remove clock gating, would you still be able to really support
> all these modes? IIUC, the clock gating is there for higher level MMC/SD
> protocols (MMC, SD, SDHC, SDXC, SDIO,...) to inform the mmc core and
> controller drivers, that according to that protocol the clock can be
> switched off now. It seems to me, that you're just completely removing
> that functionality. Neither the mmc core nor low level controller drivers
> can know when the clock can be switched off. So, I don't see how runtime
> PM by itself can solve the problem. You can choose a different approach to
> signal idle / power-saving states from protocol drivers, but you anyway
> need to actually do that. In your patch "mmc: core: Remove MMC_CLKGATE"
> however, you just remove those calls from sd.c and mmc.c, so, I don't see
> how that functionality can be implemented after your proposed patches.

Simply by using runtime PM autosuspend feature. We use a time-out
value greater then we ever will need, to complete a request according
to the MMC/SD/SDIO spec. 50 ms seems to be an accepted value here.

Kind regards
Ulf Hansson

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Guennadi Liakhovetski]

Hi Ulf

On Thu, 17 Oct 2013, Ulf Hansson wrote:

> >> > Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
> >> > tmio), I realize that they use runtime PM in an awkward way.
> >
> > I'm not sure what exactly you find awkward there. I know that a lot of
> > work has been invested in runtime PM and clock gating on those two
> > drivers, and the results were rather good: we were able to runtime suspend
> > and wake up controllers according to .power_mode as provided to drivers'
> > .set_ios() methods. We were able to use power domains and show, that even
> > when the whole domain is powered up and down as a result of our runtime
> > PM, we still can communicate with MMC, SD, and SDIO cards in slots. We
> > added support for regulators. We also added support for aggressive MMC
> > clock gating, which allowed us to save power by gating the clock _when_ it
> > was safe, as decided by higher level protocols.
> 
> By comparing with other mmc host drivers you see that the runtime PM
> implementation differs significantly for sh_mmcif and sh_mobile_sdhci.
> I am not saying it is wrong, just question it to try to understand
> why. And maybe we can improve something as well.
> 
> >
> > E.g. look at this comment in tmio_mmc_pio.c:
> >
> > /*
> >  * We differentiate between the following 3 power states:
> >  * 1. card slot powered off, controller stopped. This is used, when either there
> >  *    is no card in the slot, or the card really has to be powered down.
> 
> I guess the "card slot" be interpreted as the core power to the card, VCC/VDD?

Yes, that's right.

> >  * 2. card slot powered on, controller stopped. This is used, when a card is in
> >  *    the slot, but no activity is currently taking place. This is a power-
> >  *    saving mode with card-state preserved. This state can be entered, e.g.
> >  *    when MMC clock-gating is used.
> >  * 3. card slot powered on, controller running. This is the actual active state.
> >  */
> 
> Some questions/statements, please shoot them down if possible. :-)
> 
> 1.
> Is the controller involved in providing the actual power to the card
> (VCC/VDD)?

Not really, all is done, using external regulator drivers, even if the 
regulator and the SD/MMC controller are parts of the same SoC. But no, 
there are no registers in the SD/MMC controller itself that regulate power 
supply to the cards.

> Looking at the code in .set_ios for sh_mmcif, it seems like
> the external regulator (mmc->supply.vmmc) is the one that handles this
> on it's own. Especially since power state 3) exists as well.
> 
> If my assumption is correct, my best guess is that we shall be able to
> rework the runtime PM support as my patchset for sh_mmcif tried to do.
> Unless I missed something and there are some other obstacles to
> consider from a power domain point of view.
> 
> 2.
> Regarding clock handling. Are there any specific reason to why you
> prevent the power domain (though runtime PM) from being dropped while
> the clock is enabled (through clk_enable)? That also seems like
> unnecessary.

I'm not quite sure which code path you mean, but normally if you have the 
clock running, this means, that you want to be able to communicate with 
the card, i.e. you need power too. And if you don't need to access the 
card, then you can disable the clock and power down the card.

> > enum tmio_mmc_power {
> >         TMIO_MMC_OFF_STOP,      /* card power off, controller stopped */
> >         TMIO_MMC_ON_STOP,       /* card power on, controller stopped */
> >         TMIO_MMC_ON_RUN,        /* card power on, controller running */
> > };
> >
> > Now, if you remove clock gating, would you still be able to really support
> > all these modes? IIUC, the clock gating is there for higher level MMC/SD
> > protocols (MMC, SD, SDHC, SDXC, SDIO,...) to inform the mmc core and
> > controller drivers, that according to that protocol the clock can be
> > switched off now. It seems to me, that you're just completely removing
> > that functionality. Neither the mmc core nor low level controller drivers
> > can know when the clock can be switched off. So, I don't see how runtime
> > PM by itself can solve the problem. You can choose a different approach to
> > signal idle / power-saving states from protocol drivers, but you anyway
> > need to actually do that. In your patch "mmc: core: Remove MMC_CLKGATE"
> > however, you just remove those calls from sd.c and mmc.c, so, I don't see
> > how that functionality can be implemented after your proposed patches.
> 
> Simply by using runtime PM autosuspend feature. We use a time-out
> value greater then we ever will need, to complete a request according
> to the MMC/SD/SDIO spec. 50 ms seems to be an accepted value here.

Well, I'm not sufficiently familiar with all SD/MMC protocols to judge, 
whether there can be any states, when you need your clock running for an 
indefinitely long time. Maybe you're right and we can enter the gated 
clock state upon the autosuspend timer expiration for them. In the above 
example you'd then enter the TMIO_MMC_ON_STOP state upon such an 
autosuspend. And TMIO_MMC_OFF_STOP we would be entering upon an explicit 
call to .set_ios() with ios->power_mode == MMC_POWER_OFF. We would also 
have to take care not to use autosuspend for SDIO cards similarly to how 
the clock gating is currently disabled for them. If that's what you mean 
and if indeed we're sure, that there are no clock-on requirements during 
long inactivity periods for memory cards, then yes, I think just combining 
MMC_POWER_OFF and runtime PM autosuspend could provide the same level of 
flexibility, that the clock gating is currently providing. And even if 
there are such periods, protocol drivers can always increment the card's 
runtime PM use-count by calling mmc_get_card().

So, at the very least, I think, we should prevent SD host controllers from 
autosuspending, if an SDIO card is in the slot. Does your patch "[PATCH 
2/2] mmc: core: Remove MMC_CLKGATE" do that or do you think each SD host 
controller driver should do that by itself?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/2] mmc: core: Remove all code related to MMC_CLKGATE

[Ulf Hansson]

Hi Guennadi,

Thanks for you response!

On 17 October 2013 12:59, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Ulf
>
> On Thu, 17 Oct 2013, Ulf Hansson wrote:
>
>> >> > Looking into your two examples, sh_mmcif and sh_mobile_sdhci (using
>> >> > tmio), I realize that they use runtime PM in an awkward way.
>> >
>> > I'm not sure what exactly you find awkward there. I know that a lot of
>> > work has been invested in runtime PM and clock gating on those two
>> > drivers, and the results were rather good: we were able to runtime suspend
>> > and wake up controllers according to .power_mode as provided to drivers'
>> > .set_ios() methods. We were able to use power domains and show, that even
>> > when the whole domain is powered up and down as a result of our runtime
>> > PM, we still can communicate with MMC, SD, and SDIO cards in slots. We
>> > added support for regulators. We also added support for aggressive MMC
>> > clock gating, which allowed us to save power by gating the clock _when_ it
>> > was safe, as decided by higher level protocols.
>>
>> By comparing with other mmc host drivers you see that the runtime PM
>> implementation differs significantly for sh_mmcif and sh_mobile_sdhci.
>> I am not saying it is wrong, just question it to try to understand
>> why. And maybe we can improve something as well.
>>
>> >
>> > E.g. look at this comment in tmio_mmc_pio.c:
>> >
>> > /*
>> >  * We differentiate between the following 3 power states:
>> >  * 1. card slot powered off, controller stopped. This is used, when either there
>> >  *    is no card in the slot, or the card really has to be powered down.
>>
>> I guess the "card slot" be interpreted as the core power to the card, VCC/VDD?
>
> Yes, that's right.
>
>> >  * 2. card slot powered on, controller stopped. This is used, when a card is in
>> >  *    the slot, but no activity is currently taking place. This is a power-
>> >  *    saving mode with card-state preserved. This state can be entered, e.g.
>> >  *    when MMC clock-gating is used.
>> >  * 3. card slot powered on, controller running. This is the actual active state.
>> >  */
>>
>> Some questions/statements, please shoot them down if possible. :-)
>>
>> 1.
>> Is the controller involved in providing the actual power to the card
>> (VCC/VDD)?
>
> Not really, all is done, using external regulator drivers, even if the
> regulator and the SD/MMC controller are parts of the same SoC. But no,
> there are no registers in the SD/MMC controller itself that regulate power
> supply to the cards.
>
>> Looking at the code in .set_ios for sh_mmcif, it seems like
>> the external regulator (mmc->supply.vmmc) is the one that handles this
>> on it's own. Especially since power state 3) exists as well.
>>
>> If my assumption is correct, my best guess is that we shall be able to
>> rework the runtime PM support as my patchset for sh_mmcif tried to do.
>> Unless I missed something and there are some other obstacles to
>> consider from a power domain point of view.
>>
>> 2.
>> Regarding clock handling. Are there any specific reason to why you
>> prevent the power domain (though runtime PM) from being dropped while
>> the clock is enabled (through clk_enable)? That also seems like
>> unnecessary.
>
> I'm not quite sure which code path you mean, but normally if you have the
> clock running, this means, that you want to be able to communicate with
> the card, i.e. you need power too. And if you don't need to access the
> card, then you can disable the clock and power down the card.

I was thinking of the .set_ios function.

It is from here you do clk_enable|disable. We shall handle this from
the runtime callbacks instead. The clock needs to be enabled when
communicating with card, and if we make sure to fetch a
"runtime reference" (pm_runtime_get) during request handling, this
will fix this for us.

So, this will mean a great improvement since earlier you were always
keeping the clock enabled for the entire time the card was powered,
unless you used MMC_CLKGATE.

>
>> > enum tmio_mmc_power {
>> >         TMIO_MMC_OFF_STOP,      /* card power off, controller stopped */
>> >         TMIO_MMC_ON_STOP,       /* card power on, controller stopped */
>> >         TMIO_MMC_ON_RUN,        /* card power on, controller running */
>> > };
>> >
>> > Now, if you remove clock gating, would you still be able to really support
>> > all these modes? IIUC, the clock gating is there for higher level MMC/SD
>> > protocols (MMC, SD, SDHC, SDXC, SDIO,...) to inform the mmc core and
>> > controller drivers, that according to that protocol the clock can be
>> > switched off now. It seems to me, that you're just completely removing
>> > that functionality. Neither the mmc core nor low level controller drivers
>> > can know when the clock can be switched off. So, I don't see how runtime
>> > PM by itself can solve the problem. You can choose a different approach to
>> > signal idle / power-saving states from protocol drivers, but you anyway
>> > need to actually do that. In your patch "mmc: core: Remove MMC_CLKGATE"
>> > however, you just remove those calls from sd.c and mmc.c, so, I don't see
>> > how that functionality can be implemented after your proposed patches.
>>
>> Simply by using runtime PM autosuspend feature. We use a time-out
>> value greater then we ever will need, to complete a request according
>> to the MMC/SD/SDIO spec. 50 ms seems to be an accepted value here.
>
> Well, I'm not sufficiently familiar with all SD/MMC protocols to judge,
> whether there can be any states, when you need your clock running for an
> indefinitely long time. Maybe you're right and we can enter the gated
> clock state upon the autosuspend timer expiration for them. In the above
> example you'd then enter the TMIO_MMC_ON_STOP state upon such an
> autosuspend. And TMIO_MMC_OFF_STOP we would be entering upon an explicit
> call to .set_ios() with ios->power_mode == MMC_POWER_OFF. We would also
> have to take care not to use autosuspend for SDIO cards similarly to how
> the clock gating is currently disabled for them. If that's what you mean
> and if indeed we're sure, that there are no clock-on requirements during
> long inactivity periods for memory cards, then yes, I think just combining
> MMC_POWER_OFF and runtime PM autosuspend could provide the same level of
> flexibility, that the clock gating is currently providing. And even if
> there are such periods, protocol drivers can always increment the card's
> runtime PM use-count by calling mmc_get_card().

You are right regarding SDIO, but only in the case were the host
implements SDIO IRQ.

Though, the host must not use mmc_get_card, because that is something
completely different. mmc_get_card, make sure the card device, which
sits on the mmc_bus becomes active. It is separate from the host
device.

To solve the SDIO IRQ problem, there are two options.
1.
Keep the host device active as long as you have an SDIO IRQ enabled.
Thus do one more pm_runtime_get, on top of the others. From a power
save point of view this is not to prefer, but it works.

2. When entering runtime_suspend state, re-route the DAT1 line to a
GPIO IRQ and possibly also set it as a wakeup irq if you want to
listen on it during in system suspended state. In this case you are
able to fetch the SDIO IRQ, even if the clock and controller are
sleeping. Once woken up, in other words, being runtime_resumed, the
GPIO IRQ is detached.

So to conclude, since sh_mmcif, don't implement SDIO IRQ, we don't
need any adaptations for SDIO.

Kind regards
Uffe

>
> So, at the very least, I think, we should prevent SD host controllers from
> autosuspending, if an SDIO card is in the slot. Does your patch "[PATCH
> 2/2] mmc: core: Remove MMC_CLKGATE" do that or do you think each SD host
> controller driver should do that by itself?
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/