[RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Jaehoon Chung]

Hi all,
  This is a RFC patch that support clock-gating for saving power consumption.
  I found mmc_host_enable/mmc_host_disable function in core.c
  (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
  So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c

i want any feedback. how do you think about this patch?
Plz let me know...

Thank you all

 Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
 Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

---
 drivers/mmc/host/sdhci-s3c.c |   36 ++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 71ad416..9b430fb 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -47,6 +47,7 @@ struct sdhci_s3c {
 	unsigned int		cur_clk;
 	int			ext_cd_irq;
 	int			ext_cd_gpio;
+	int			flag;
 
 	struct clk		*clk_io;
 	struct clk		*clk_bus[MAX_BUS_CLK];
@@ -232,10 +233,45 @@ static unsigned int sdhci_s3c_get_min_clock(struct sdhci_host *host)
 	return min;
 }
 
+static int sdhci_s3c_enable_clk(struct sdhci_host *host)
+{
+	struct sdhci_s3c *sc = to_s3c(host);
+
+	if (sc->flag != 1) {
+		clk_disable(sc->clk_io);
+		clk_disable(sc->clk_bus[sc->cur_clk]);
+	}
+
+	if (sc->host->clk_cnt == 0) {
+		clk_enable(sc->clk_io);
+		clk_enable(sc->clk_bus[sc->cur_clk]);
+		sc->host->clk_cnt++;
+		sc->flag = 1;
+	}
+
+	return 0;
+}
+
+static int sdhci_s3c_disable_clk(struct sdhci_host *host, int lazy)
+{
+	struct sdhci_s3c *sc = to_s3c(host);
+
+	if (sc->host->clk_cnt) {
+		clk_disable(sc->clk_io);
+		clk_disable(sc->clk_bus[sc->cur_clk]);
+		if (sc->host->clk_cnt)
+			sc->host->clk_cnt--;
+	}
+
+	return 0;
+}
+
 static struct sdhci_ops sdhci_s3c_ops = {
 	.get_max_clock		= sdhci_s3c_get_max_clk,
 	.set_clock		= sdhci_s3c_set_clock,
 	.get_min_clock		= sdhci_s3c_get_min_clock,
+	.enable			= sdhci_s3c_enable_clk,
+	.disable		= sdhci_s3c_disable_clk,
 };
 
 static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 401527d..fa2e55d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1245,7 +1245,37 @@ out:
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
+static int sdhci_enable_clk(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int ret = 0;
+
+	if (host->old_clock != 0 && host->clock == 0) {
+		if (host->ops->enable)
+			ret = host->ops->enable(host);
+		sdhci_set_clock(host, host->old_clock);
+	}
+
+	return ret;
+}
+
+static int sdhci_disable_clk(struct mmc_host *mmc, int lazy)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int ret = 0;
+
+	if (host->clock != 0) {
+		host->old_clock = host->clock;
+		sdhci_set_clock(host, 0);
+		if (host->ops->disable)
+			ret = host->ops->disable(host, lazy);
+	}
+	return ret;
+}
+
 static const struct mmc_host_ops sdhci_ops = {
+	.enable		= sdhci_enable_clk,
+	.disable	= sdhci_disable_clk,
 	.request	= sdhci_request,
 	.set_ios	= sdhci_set_ios,
 	.get_ro		= sdhci_get_ro,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d316bc7..0c6f143 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -278,6 +278,8 @@ struct sdhci_host {
 	unsigned int		timeout_clk;	/* Timeout freq (KHz) */
 
 	unsigned int		clock;		/* Current clock (MHz) */
+	unsigned int		old_clock;	/* Old clock (MHz) */
+	unsigned int		clk_cnt;	/* Clock user count */
 	u8			pwr;		/* Current voltage */
 
 	struct mmc_request	*mrq;		/* Current request */
@@ -323,6 +325,8 @@ struct sdhci_ops {
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
 	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
+	int		(*enable)(struct sdhci_host *host);
+	int		(*disable)(struct sdhci_host *host, int lazy);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 1.6.0.4 

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Matt Fleming]

On Thu, Sep 16, 2010 at 03:46:50PM +0900, Jaehoon Chung wrote:
> Hi all,
>   This is a RFC patch that support clock-gating for saving power consumption.
>   I found mmc_host_enable/mmc_host_disable function in core.c
>   (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
>   So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c
> 
> i want any feedback. how do you think about this patch?
> Plz let me know...
> 
> Thank you all
> 
>  Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>  Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> ---
>  drivers/mmc/host/sdhci-s3c.c |   36 ++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.h     |    4 ++++
>  3 files changed, 70 insertions(+), 0 deletions(-)

[...]

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 401527d..fa2e55d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1245,7 +1245,37 @@ out:
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> +static int sdhci_enable_clk(struct mmc_host *mmc)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int ret = 0;
> +
> +	if (host->old_clock != 0 && host->clock == 0) {
> +		if (host->ops->enable)
> +			ret = host->ops->enable(host);
> +		sdhci_set_clock(host, host->old_clock);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sdhci_disable_clk(struct mmc_host *mmc, int lazy)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int ret = 0;
> +
> +	if (host->clock != 0) {
> +		host->old_clock = host->clock;
> +		sdhci_set_clock(host, 0);
> +		if (host->ops->disable)
> +			ret = host->ops->disable(host, lazy);
> +	}
> +	return ret;
> +}
> +
>  static const struct mmc_host_ops sdhci_ops = {
> +	.enable		= sdhci_enable_clk,
> +	.disable	= sdhci_disable_clk,
>  	.request	= sdhci_request,
>  	.set_ios	= sdhci_set_ios,
>  	.get_ro		= sdhci_get_ro,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d316bc7..0c6f143 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -278,6 +278,8 @@ struct sdhci_host {
>  	unsigned int		timeout_clk;	/* Timeout freq (KHz) */
>  
>  	unsigned int		clock;		/* Current clock (MHz) */
> +	unsigned int		old_clock;	/* Old clock (MHz) */
> +	unsigned int		clk_cnt;	/* Clock user count */
>  	u8			pwr;		/* Current voltage */
>  
>  	struct mmc_request	*mrq;		/* Current request */
> @@ -323,6 +325,8 @@ struct sdhci_ops {
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> +	int		(*enable)(struct sdhci_host *host);
> +	int		(*disable)(struct sdhci_host *host, int lazy);
>  };
>  
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

I could have misunderstood something, but do you really need this new
'old_clock' member? Is the previous clock value not stored in
host->ios.clock?

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Jae hoon Chung]

2010/9/17 Matt Fleming <matt@console-pimps.org>:
> On Thu, Sep 16, 2010 at 03:46:50PM +0900, Jaehoon Chung wrote:
>> Hi all,
>>   This is a RFC patch that support clock-gating for saving power consumption.
>>   I found mmc_host_enable/mmc_host_disable function in core.c
>>   (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
>>   So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c
>>
>> i want any feedback. how do you think about this patch?
>> Plz let me know...
>>
>> Thank you all
>>
>>  Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>  Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> ---
>>  drivers/mmc/host/sdhci-s3c.c |   36 ++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.h     |    4 ++++
>>  3 files changed, 70 insertions(+), 0 deletions(-)
>
> [...]
>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 401527d..fa2e55d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1245,7 +1245,37 @@ out:
>>       spin_unlock_irqrestore(&host->lock, flags);
>>  }
>>
>> +static int sdhci_enable_clk(struct mmc_host *mmc)
>> +{
>> +     struct sdhci_host *host = mmc_priv(mmc);
>> +     int ret = 0;
>> +
>> +     if (host->old_clock != 0 && host->clock == 0) {
>> +             if (host->ops->enable)
>> +                     ret = host->ops->enable(host);
>> +             sdhci_set_clock(host, host->old_clock);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int sdhci_disable_clk(struct mmc_host *mmc, int lazy)
>> +{
>> +     struct sdhci_host *host = mmc_priv(mmc);
>> +     int ret = 0;
>> +
>> +     if (host->clock != 0) {
>> +             host->old_clock = host->clock;
>> +             sdhci_set_clock(host, 0);
>> +             if (host->ops->disable)
>> +                     ret = host->ops->disable(host, lazy);
>> +     }
>> +     return ret;
>> +}
>> +
>>  static const struct mmc_host_ops sdhci_ops = {
>> +     .enable         = sdhci_enable_clk,
>> +     .disable        = sdhci_disable_clk,
>>       .request        = sdhci_request,
>>       .set_ios        = sdhci_set_ios,
>>       .get_ro         = sdhci_get_ro,
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index d316bc7..0c6f143 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -278,6 +278,8 @@ struct sdhci_host {
>>       unsigned int            timeout_clk;    /* Timeout freq (KHz) */
>>
>>       unsigned int            clock;          /* Current clock (MHz) */
>> +     unsigned int            old_clock;      /* Old clock (MHz) */
>> +     unsigned int            clk_cnt;        /* Clock user count */
>>       u8                      pwr;            /* Current voltage */
>>
>>       struct mmc_request      *mrq;           /* Current request */
>> @@ -323,6 +325,8 @@ struct sdhci_ops {
>>       unsigned int    (*get_max_clock)(struct sdhci_host *host);
>>       unsigned int    (*get_min_clock)(struct sdhci_host *host);
>>       unsigned int    (*get_timeout_clock)(struct sdhci_host *host);
>> +     int             (*enable)(struct sdhci_host *host);
>> +     int             (*disable)(struct sdhci_host *host, int lazy);
>>  };
>>
>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>
> I could have misunderstood something, but do you really need this new
> 'old_clock' member? Is the previous clock value not stored in
> host->ios.clock?

Thanks Matt for your comment.
if host->ios.clock set zero, i think that need previous value (for
example 52MHz..)
So i add 'old_clock' member...i will test, after removed 'old_clock'

anyother doubt? and problem?
Thanks

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Jaehoon Chung]

Jae hoon Chung wrote:
> 2010/9/17 Matt Fleming <matt@console-pimps.org>:
>> On Thu, Sep 16, 2010 at 03:46:50PM +0900, Jaehoon Chung wrote:
>>> Hi all,
>>>   This is a RFC patch that support clock-gating for saving power consumption.
>>>   I found mmc_host_enable/mmc_host_disable function in core.c
>>>   (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
>>>   So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c
>>>
>>> i want any feedback. how do you think about this patch?
>>> Plz let me know...
>>>
>>> Thank you all
>>>
>>>  Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>  Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>
>>> ---
>>>  drivers/mmc/host/sdhci-s3c.c |   36 ++++++++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/sdhci.h     |    4 ++++
>>>  3 files changed, 70 insertions(+), 0 deletions(-)
>> [...]
>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 401527d..fa2e55d 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1245,7 +1245,37 @@ out:
>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>  }
>>>
>>> +static int sdhci_enable_clk(struct mmc_host *mmc)
>>> +{
>>> +     struct sdhci_host *host = mmc_priv(mmc);
>>> +     int ret = 0;
>>> +
>>> +     if (host->old_clock != 0 && host->clock == 0) {
>>> +             if (host->ops->enable)
>>> +                     ret = host->ops->enable(host);
>>> +             sdhci_set_clock(host, host->old_clock);
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int sdhci_disable_clk(struct mmc_host *mmc, int lazy)
>>> +{
>>> +     struct sdhci_host *host = mmc_priv(mmc);
>>> +     int ret = 0;
>>> +
>>> +     if (host->clock != 0) {
>>> +             host->old_clock = host->clock;
>>> +             sdhci_set_clock(host, 0);
>>> +             if (host->ops->disable)
>>> +                     ret = host->ops->disable(host, lazy);
>>> +     }
>>> +     return ret;
>>> +}
>>> +
>>>  static const struct mmc_host_ops sdhci_ops = {
>>> +     .enable         = sdhci_enable_clk,
>>> +     .disable        = sdhci_disable_clk,
>>>       .request        = sdhci_request,
>>>       .set_ios        = sdhci_set_ios,
>>>       .get_ro         = sdhci_get_ro,
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index d316bc7..0c6f143 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -278,6 +278,8 @@ struct sdhci_host {
>>>       unsigned int            timeout_clk;    /* Timeout freq (KHz) */
>>>
>>>       unsigned int            clock;          /* Current clock (MHz) */
>>> +     unsigned int            old_clock;      /* Old clock (MHz) */
>>> +     unsigned int            clk_cnt;        /* Clock user count */
>>>       u8                      pwr;            /* Current voltage */
>>>
>>>       struct mmc_request      *mrq;           /* Current request */
>>> @@ -323,6 +325,8 @@ struct sdhci_ops {
>>>       unsigned int    (*get_max_clock)(struct sdhci_host *host);
>>>       unsigned int    (*get_min_clock)(struct sdhci_host *host);
>>>       unsigned int    (*get_timeout_clock)(struct sdhci_host *host);
>>> +     int             (*enable)(struct sdhci_host *host);
>>> +     int             (*disable)(struct sdhci_host *host, int lazy);
>>>  };
>>>
>>>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> I could have misunderstood something, but do you really need this new
>> 'old_clock' member? Is the previous clock value not stored in
>> host->ios.clock?
> 
> Thanks Matt for your comment.
> if host->ios.clock set zero, i think that need previous value (for
> example 52MHz..)
> So i add 'old_clock' member...i will test, after removed 'old_clock'
> 
> anyother doubt? and problem?
> Thanks
> 
Hi Chris.
I want to know how do you think about this patch.
If you have any doubt in this patch, let me know plz.

Thanks
Jaehoon Chung

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Chris Ball]

Hi Jaehoon/Adrian,

On Thu, Sep 16, 2010 at 03:46:50PM +0900, Jaehoon Chung wrote:
> Hi all,
>   This is a RFC patch that support clock-gating for saving power consumption.
>   I found mmc_host_enable/mmc_host_disable function in core.c
>   (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
>   So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c
> 
> i want any feedback. how do you think about this patch?
> Plz let me know...

A few points:
  * Have you tested this patch?  Did you see a decrease in power
    consumption?  How large was the decrease?
  * I don't understand exactly how/when you're expecting to save power
    with this approach of defining .{enable,disable}() without then
    calling them from your driver code.  Under which circumstances do
    you think this will power down the clock?
  * CC'ing Adrian for help with review, since he wrote these callbacks.

Thanks,

- Chris.

> Thank you all
> 
>  Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>  Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> ---
>  drivers/mmc/host/sdhci-s3c.c |   36 ++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.h     |    4 ++++
>  3 files changed, 70 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 71ad416..9b430fb 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -47,6 +47,7 @@ struct sdhci_s3c {
>  	unsigned int		cur_clk;
>  	int			ext_cd_irq;
>  	int			ext_cd_gpio;
> +	int			flag;
>  
>  	struct clk		*clk_io;
>  	struct clk		*clk_bus[MAX_BUS_CLK];
> @@ -232,10 +233,45 @@ static unsigned int sdhci_s3c_get_min_clock(struct sdhci_host *host)
>  	return min;
>  }
>  
> +static int sdhci_s3c_enable_clk(struct sdhci_host *host)
> +{
> +	struct sdhci_s3c *sc = to_s3c(host);
> +
> +	if (sc->flag != 1) {
> +		clk_disable(sc->clk_io);
> +		clk_disable(sc->clk_bus[sc->cur_clk]);
> +	}
> +
> +	if (sc->host->clk_cnt == 0) {
> +		clk_enable(sc->clk_io);
> +		clk_enable(sc->clk_bus[sc->cur_clk]);
> +		sc->host->clk_cnt++;
> +		sc->flag = 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sdhci_s3c_disable_clk(struct sdhci_host *host, int lazy)
> +{
> +	struct sdhci_s3c *sc = to_s3c(host);
> +
> +	if (sc->host->clk_cnt) {
> +		clk_disable(sc->clk_io);
> +		clk_disable(sc->clk_bus[sc->cur_clk]);
> +		if (sc->host->clk_cnt)
> +			sc->host->clk_cnt--;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct sdhci_ops sdhci_s3c_ops = {
>  	.get_max_clock		= sdhci_s3c_get_max_clk,
>  	.set_clock		= sdhci_s3c_set_clock,
>  	.get_min_clock		= sdhci_s3c_get_min_clock,
> +	.enable			= sdhci_s3c_enable_clk,
> +	.disable		= sdhci_s3c_disable_clk,
>  };
>  
>  static void sdhci_s3c_notify_change(struct platform_device *dev, int state)
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 401527d..fa2e55d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1245,7 +1245,37 @@ out:
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
> +static int sdhci_enable_clk(struct mmc_host *mmc)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int ret = 0;
> +
> +	if (host->old_clock != 0 && host->clock == 0) {
> +		if (host->ops->enable)
> +			ret = host->ops->enable(host);
> +		sdhci_set_clock(host, host->old_clock);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sdhci_disable_clk(struct mmc_host *mmc, int lazy)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int ret = 0;
> +
> +	if (host->clock != 0) {
> +		host->old_clock = host->clock;
> +		sdhci_set_clock(host, 0);
> +		if (host->ops->disable)
> +			ret = host->ops->disable(host, lazy);
> +	}
> +	return ret;
> +}
> +
>  static const struct mmc_host_ops sdhci_ops = {
> +	.enable		= sdhci_enable_clk,
> +	.disable	= sdhci_disable_clk,
>  	.request	= sdhci_request,
>  	.set_ios	= sdhci_set_ios,
>  	.get_ro		= sdhci_get_ro,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index d316bc7..0c6f143 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -278,6 +278,8 @@ struct sdhci_host {
>  	unsigned int		timeout_clk;	/* Timeout freq (KHz) */
>  
>  	unsigned int		clock;		/* Current clock (MHz) */
> +	unsigned int		old_clock;	/* Old clock (MHz) */
> +	unsigned int		clk_cnt;	/* Clock user count */
>  	u8			pwr;		/* Current voltage */
>  
>  	struct mmc_request	*mrq;		/* Current request */
> @@ -323,6 +325,8 @@ struct sdhci_ops {
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_timeout_clock)(struct sdhci_host *host);
> +	int		(*enable)(struct sdhci_host *host);
> +	int		(*disable)(struct sdhci_host *host, int lazy);
>  };
>  
>  #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> -- 1.6.0.4 

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

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Nicolas Pitre]

On Thu, 30 Sep 2010, Chris Ball wrote:

> Hi Jaehoon/Adrian,
> 
> On Thu, Sep 16, 2010 at 03:46:50PM +0900, Jaehoon Chung wrote:
> > Hi all,
> >   This is a RFC patch that support clock-gating for saving power consumption.
> >   I found mmc_host_enable/mmc_host_disable function in core.c
> >   (using MMC_CAP_DSIABLE. i think that use when host enable/disable)
> >   So, i used that functions and implemented some functions in sdhci-s3c.c & sdhci.c
> > 
> > i want any feedback. how do you think about this patch?
> > Plz let me know...
> 
> A few points:
>   * Have you tested this patch?  Did you see a decrease in power
>     consumption?  How large was the decrease?
>   * I don't understand exactly how/when you're expecting to save power
>     with this approach of defining .{enable,disable}() without then
>     calling them from your driver code.  Under which circumstances do
>     you think this will power down the clock?
>   * CC'ing Adrian for help with review, since he wrote these callbacks.

As I already said here: 

	http://article.gmane.org/gmane.linux.ports.arm.omap/39411

I find those callbacks rather problematic.  Currently, 
mmc_host_disable() is called by the host driver (currently OMAP) and 
that's wrong.  Such decision cannot be made in the controller driver -- 
it has to be made higher up the stack.


Nicolas

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Chris Ball]

Hi,

On Wed, Sep 29, 2010 at 11:42:01PM -0400, Nicolas Pitre wrote:
> As I already said here: 
> 
> 	http://article.gmane.org/gmane.linux.ports.arm.omap/39411
> 
> I find those callbacks rather problematic.  Currently, 
> mmc_host_disable() is called by the host driver (currently OMAP) and 
> that's wrong.  Such decision cannot be made in the controller driver -- 
> it has to be made higher up the stack.

I strongly agree.  I hadn't noticed that aspect of this design until
today.  It looked like Linus W had a nice core-integrated clocking
framework almost ready to go a year ago, but it lost out.  (Something
left to do was to give extra time after a request in case we're on a
broken card which requires the card clock to be present during its
writeback.)

I'm not sure where to go from here -- advice welcome.  It would
perhaps be ideal if Linus W and Adrian could work together to make
the current framework look more like Linus' original intent and then
move omap_hsmmc to it as painlessly as possible.  Of course I wouldn't
expect this to happen quickly.

In the meantime, I would suggest that we should not accept any more
users of this framework, which would be a NACK to Jaehoon's patch
(which appears to my reading not to achieve any power-saving anyway).

Adrian, I'm sorry that I'm suggesting reverting -- or at least strongly
modifying -- a framework after it's already shipped in a release; if
you think this is unreasonable, I'll consider your argument carefully.

Thanks,

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

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Kyungmin Park]

On Thu, Sep 30, 2010 at 1:09 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Wed, Sep 29, 2010 at 11:42:01PM -0400, Nicolas Pitre wrote:
>> As I already said here:
>>
>>       http://article.gmane.org/gmane.linux.ports.arm.omap/39411
>>
>> I find those callbacks rather problematic.  Currently,
>> mmc_host_disable() is called by the host driver (currently OMAP) and
>> that's wrong.  Such decision cannot be made in the controller driver --
>> it has to be made higher up the stack.
>
> I strongly agree.  I hadn't noticed that aspect of this design until
> today.  It looked like Linus W had a nice core-integrated clocking
> framework almost ready to go a year ago, but it lost out.  (Something
> left to do was to give extra time after a request in case we're on a
> broken card which requires the card clock to be present during its
> writeback.)

Do you mean this one?
http://www.mail-archive.com/linux-embedded@vger.kernel.org/msg01883.html

In previous time it used for our kernel and find it's difficult to
handle and maintain and can't support the SD card.
So try to find another method and implemente it at SDHCI drivers.

>
> I'm not sure where to go from here -- advice welcome.  It would
> perhaps be ideal if Linus W and Adrian could work together to make
> the current framework look more like Linus' original intent and then
> move omap_hsmmc to it as painlessly as possible.  Of course I wouldn't
> expect this to happen quickly.
>
> In the meantime, I would suggest that we should not accept any more
> users of this framework, which would be a NACK to Jaehoon's patch
> (which appears to my reading not to achieve any power-saving anyway).

No, In our case it deduce the about 10mA. so it's mandatory and basic
functionality to reduce the power.

Thank you,
Kyungmin Park
>
> Adrian, I'm sorry that I'm suggesting reverting -- or at least strongly
> modifying -- a framework after it's already shipped in a release; if
> you think this is unreasonable, I'll consider your argument carefully.
>
> Thanks,
>
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> 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
>

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Linus Walleij]

2010/9/30 Kyungmin Park <kyungmin.park@samsung.com>:
> On Thu, Sep 30, 2010 at 1:09 PM, Chris Ball <cjb@laptop.org> wrote:
>> Hi,
>>
>> On Wed, Sep 29, 2010 at 11:42:01PM -0400, Nicolas Pitre wrote:
>>> As I already said here:
>>>
>>>       http://article.gmane.org/gmane.linux.ports.arm.omap/39411
>>>
>>> I find those callbacks rather problematic.  Currently,
>>> mmc_host_disable() is called by the host driver (currently OMAP) and
>>> that's wrong.  Such decision cannot be made in the controller driver --
>>> it has to be made higher up the stack.
>>
>> I strongly agree.  I hadn't noticed that aspect of this design until
>> today.  It looked like Linus W had a nice core-integrated clocking
>> framework almost ready to go a year ago, but it lost out.  (Something
>> left to do was to give extra time after a request in case we're on a
>> broken card which requires the card clock to be present during its
>> writeback.)
>
> Do you mean this one?
> http://www.mail-archive.com/linux-embedded@vger.kernel.org/msg01883.html

That's it. If there is interest I can surely pick it up and wrap the
current enable/disable calls around it or even rewrite all drivers
to use the set_ios() approach as in this patchset I think.

What happened was that I had this iterated a few times as Pierre was
getting increasingly overloaded and it was on the verge of merging
when he resigned as MMC maintainer.

The next week or so the big hammer callbacks from OMAP were
merged as part of their mainline procedure.

> In previous time it used for our kernel and find it's difficult to
> handle and maintain and can't support the SD card.
> So try to find another method and implemente it at SDHCI drivers.

Why? The only difference between this solution and the one
that is inside the kernel now is that this will use set_ios() with
a zero argument to gate off the clock after a timeout instead
of special callbacks. (A features especially requested by Pierre.)

I can move the timeing code to wrap the current callbacks
instead atleast that would make things more according to spec.

For MMC cards that is.

For SD cards IIRC the spec is the same, but in practice all
SD cards (and especially all MicroSD and eMMCs we see these
days) survive gating the clock off like these current callbacks do.
That I believe is why people have so little trouble with the present
code.

Yours,
Linus Walleij

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Nicolas Pitre]

On Thu, 30 Sep 2010, Linus Walleij wrote:

> 2010/9/30 Kyungmin Park <kyungmin.park@samsung.com>:
> > So try to find another method and implemente it at SDHCI drivers.
> 
> Why? The only difference between this solution and the one
> that is inside the kernel now is that this will use set_ios() with
> a zero argument to gate off the clock after a timeout instead
> of special callbacks. (A features especially requested by Pierre.)

Good!  I didn't look at your solution before, but in my mind that always 
been how it should be done.


Nicolas

Re: [RFC RESEND] sdhci-s3c: support clock enable/disable (clock-gating)

[Chris Ball]

Hi Linus,

On Thu, Sep 30, 2010 at 09:22:17AM +0200, Linus Walleij wrote:
> That's it. If there is interest I can surely pick it up and wrap the
> current enable/disable calls around it or even rewrite all drivers
> to use the set_ios() approach as in this patchset I think.
> 
> What happened was that I had this iterated a few times as Pierre was
> getting increasingly overloaded and it was on the verge of merging
> when he resigned as MMC maintainer.
> 
> The next week or so the big hammer callbacks from OMAP were
> merged as part of their mainline procedure.
> 
> I can move the timeing code to wrap the current callbacks
> instead atleast that would make things more according to spec.

Sounds excellent.  Thank you!

There are a few groups to keep in touch with on this:

* The omap_hsmmc folks, of course
* Ohad Ben-Cohen has SDIO runtime PM patches¹, which I think should
  be merged
* Kevin Hilman is working on converting omap_hsmmc over to runtime PM²
* Matthew Garrett is working on a patch to power down unused SDHCI
  devices using runtime PM

- Chris.

¹:  http://thread.gmane.org/gmane.linux.kernel.mmc/3968
²:  http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/pm-wip/mmc

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