[PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Guennadi Liakhovetski]

Hi all

I've already sent one of these two patches yesterday, but I forgot to 
include Rafael on the CC list and forgot to mention, that the patch should 
be treated as a regression fix and should go into 3.3. The reason why the 
commit

commit 7e09bedba1b87f9c7b34ba895b57baf0c36ccdc8
Author: Sujit Reddy Thumma <sthumma@codeaurora.org>
Date:   Mon Nov 14 13:53:29 2011 +0530

    mmc: core: Use delayed work in clock gating framework

can be considered a regression is, that it extends the powered-on time of 
the controller(s) and, possibly, of respective power domains by 200ms (by 
default), which is an essential change in behaviour in the negative 
direction.

However, if this is indeed the desired change, maybe the better approach 
now would be to modify this behaviour in individual drivers, which is 
exactly what the two patches in this series are doing. The tmio_mmc patch 
is just a resend from yesterday:

http://article.gmane.org/gmane.linux.kernel.mmc/12270

- repeated only for added CC and for easier review.

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

[PATCH 1/2] mmc: tmio_mmc: use PM QoS instead of delayed clock gating for PM

[Guennadi Liakhovetski]

Using delayed clock gating to prevent too frequent gating of the clock
is simple and efficient, but too inflexible. We use PM QoS instead to
let the runtime PM subsystem decide, which power states can be entered
at any specific time, depending on the currently active governor.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

An exact resend of the yesterday's patch

http://article.gmane.org/gmane.linux.kernel.mmc/12270

 drivers/mmc/host/tmio_mmc.h     |    2 ++
 drivers/mmc/host/tmio_mmc_pio.c |   21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index b4519bf..8afaf4f 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -20,6 +20,7 @@
 #include <linux/mmc/tmio.h>
 #include <linux/mutex.h>
 #include <linux/pagemap.h>
+#include <linux/pm_qos.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
 
@@ -50,6 +51,7 @@ struct tmio_mmc_host {
 
 	/* Controller power state */
 	bool			power;
+	struct dev_pm_qos_request pm_qos;
 
 	/* Callbacks for clock / power control */
 	void (*set_pwr)(struct platform_device *host, int state);
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 0e72f1a..576baf5 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -40,6 +40,7 @@
 #include <linux/module.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
@@ -853,11 +854,27 @@ static int tmio_mmc_get_cd(struct mmc_host *mmc)
 		return pdata->get_cd(host->pdev);
 }
 
+static int tmio_mmc_enable(struct mmc_host *mmc)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	dev_pm_qos_add_request(mmc->parent, &host->pm_qos, 100);
+	return 0;
+}
+
+static int tmio_mmc_disable(struct mmc_host *mmc, int lazy)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	dev_pm_qos_remove_request(&host->pm_qos);
+	return 0;
+}
+
 static const struct mmc_host_ops tmio_mmc_ops = {
 	.request	= tmio_mmc_request,
 	.set_ios	= tmio_mmc_set_ios,
 	.get_ro         = tmio_mmc_get_ro,
 	.get_cd		= tmio_mmc_get_cd,
+	.enable		= tmio_mmc_enable,
+	.disable	= tmio_mmc_disable,
 	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
 };
 
@@ -879,6 +896,8 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	if (!mmc)
 		return -ENOMEM;
 
+	/* disable delayed clock gating, we use PM QoS for precise PM */
+	mmc->clkgate_delay = 0;
 	pdata->dev = &pdev->dev;
 	_host = mmc_priv(mmc);
 	_host->pdata = pdata;
@@ -899,7 +918,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	}
 
 	mmc->ops = &tmio_mmc_ops;
-	mmc->caps = MMC_CAP_4_BIT_DATA | pdata->capabilities;
+	mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_DISABLE | pdata->capabilities;
 	if (pdata->get_clk_rate)
 		mmc->f_max = pdata->get_clk_rate(pdev);
 	else
-- 
1.7.2.5

[PATCH 2/2] mmc: sh_mmcif: use PM QoS instead of delayed clock gating for PM

[Guennadi Liakhovetski]

Using delayed clock gating to prevent too frequent gating of the clock
is simple and efficient, but too inflexible. We use PM QoS instead to
let the runtime PM subsystem decide, which power states can be entered
at any specific time, depending on the currently active governor.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/mmc/host/sh_mmcif.c |   27 ++++++++++++++++++++++++---
 1 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 497e6f4..5a922be 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -55,6 +55,7 @@
 #include <linux/mmc/sh_mmcif.h>
 #include <linux/pagemap.h>
 #include <linux/platform_device.h>
+#include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/module.h>
@@ -226,6 +227,7 @@ struct sh_mmcif_host {
 	size_t blocksize;
 	int sg_idx;
 	int sg_blkidx;
+	struct dev_pm_qos_request pm_qos;
 	bool power;
 	bool card_present;
 
@@ -977,10 +979,26 @@ static int sh_mmcif_get_cd(struct mmc_host *mmc)
 		return p->get_cd(host->pd);
 }
 
+static int sh_mmcif_enable(struct mmc_host *mmc)
+{
+	struct sh_mmcif_host *host = mmc_priv(mmc);
+	dev_pm_qos_add_request(mmc->parent, &host->pm_qos, 100);
+	return 0;
+}
+
+static int sh_mmcif_disable(struct mmc_host *mmc, int lazy)
+{
+	struct sh_mmcif_host *host = mmc_priv(mmc);
+	dev_pm_qos_remove_request(&host->pm_qos);
+	return 0;
+}
+
 static struct mmc_host_ops sh_mmcif_ops = {
 	.request	= sh_mmcif_request,
 	.set_ios	= sh_mmcif_set_ios,
 	.get_cd		= sh_mmcif_get_cd,
+	.enable		= sh_mmcif_enable,
+	.disable	= sh_mmcif_disable,
 };
 
 static bool sh_mmcif_end_cmd(struct sh_mmcif_host *host)
@@ -1061,7 +1079,6 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
 {
 	struct sh_mmcif_host *host = dev_id;
 	struct mmc_request *mrq = host->mrq;
-	struct mmc_data *data = mrq->data;
 
 	cancel_delayed_work_sync(&host->timeout_work);
 
@@ -1109,13 +1126,15 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
 	case MMCIF_WAIT_FOR_READ_END:
 	case MMCIF_WAIT_FOR_WRITE_END:
 		if (host->sd_error)
-			data->error = sh_mmcif_error_manage(host);
+			mrq->data->error = sh_mmcif_error_manage(host);
 		break;
 	default:
 		BUG();
 	}
 
 	if (host->wait_for != MMCIF_WAIT_FOR_STOP) {
+		struct mmc_data *data = mrq->data;
+
 		if (!mrq->cmd->error && data && !data->error)
 			data->bytes_xfered  				data->blocks * data->blksz;
@@ -1303,12 +1322,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
 		mmc->f_min = mmc->f_max / 512;
 	if (pd->ocr)
 		mmc->ocr_avail = pd->ocr;
-	mmc->caps = MMC_CAP_MMC_HIGHSPEED | pd->caps;
+	mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_DISABLE | pd->caps;
 	mmc->max_segs = 32;
 	mmc->max_blk_size = 512;
 	mmc->max_req_size = PAGE_CACHE_SIZE * mmc->max_segs;
 	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
 	mmc->max_seg_size = mmc->max_req_size;
+	/* disable delayed clock gating, we use PM QoS for precise PM */
+	mmc->clkgate_delay = 0;
 
 	sh_mmcif_sync_reset(host);
 	platform_set_drvdata(pdev, host);
-- 
1.7.2.5

Re: [PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Rafael J. Wysocki]

Hi,

On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> Hi all
> 
> I've already sent one of these two patches yesterday, but I forgot to 
> include Rafael on the CC list and forgot to mention, that the patch should 
> be treated as a regression fix and should go into 3.3. The reason why the 
> commit
> 
> commit 7e09bedba1b87f9c7b34ba895b57baf0c36ccdc8

I think that should be 597dd9d79cfbbb1636d00a7fd0880355d9b20c41, right?

> Author: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Date:   Mon Nov 14 13:53:29 2011 +0530
> 
>     mmc: core: Use delayed work in clock gating framework
> 
> can be considered a regression is, that it extends the powered-on time of 
> the controller(s) and, possibly, of respective power domains by 200ms (by 
> default), which is an essential change in behaviour in the negative 
> direction.
> 
> However, if this is indeed the desired change, maybe the better approach 
> now would be to modify this behaviour in individual drivers, which is 
> exactly what the two patches in this series are doing.

I agree that doing such things at the framework level is not a good idea,
which this example clearly shows.

The commit above had made the assumption that every platform would
regard performance as more important than power saving and made a wholesale
change that affected everyone.  In some cases, however, the change is not
welcome and is not too easy to work around (because of the user space interface
file added in the process).

Quite frankly, I think it would be better to simply revert that commit (along
with the later one fixing it) at this point, because there may be more cases
where it actually makes things worse.

As for the patches, I'll reply to each of them separately.

Thanks,
Rafael

Re: [PATCH 1/2] mmc: tmio_mmc: use PM QoS instead of delayed clock gating for PM

[Rafael J. Wysocki]

Hi,

On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> Using delayed clock gating to prevent too frequent gating of the clock
> is simple and efficient, but too inflexible.

I'd mention that you're addressing an undesirable effect of commit
597dd9d79cfbbb1636d00a7fd0880355d9b20c41 here.

> We use PM QoS instead to
> let the runtime PM subsystem decide, which power states can be entered
> at any specific time, depending on the currently active governor.

Do I think correctly that this is going to work because tmio_mmc_set_ios()
calls pm_runtime_get_sync() if power_mode is MMC_POWER_ON and
pm_runtime_put() otherwise?

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> An exact resend of the yesterday's patch
> 
> http://article.gmane.org/gmane.linux.kernel.mmc/12270
> 
>  drivers/mmc/host/tmio_mmc.h     |    2 ++
>  drivers/mmc/host/tmio_mmc_pio.c |   21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index b4519bf..8afaf4f 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -20,6 +20,7 @@
>  #include <linux/mmc/tmio.h>
>  #include <linux/mutex.h>
>  #include <linux/pagemap.h>
> +#include <linux/pm_qos.h>
>  #include <linux/scatterlist.h>
>  #include <linux/spinlock.h>
>  
> @@ -50,6 +51,7 @@ struct tmio_mmc_host {
>  
>  	/* Controller power state */
>  	bool			power;

My version of struct tmio_mmc_host (current mainline) doesn't include the
power member.  Perhaps the patch should be rebased on top of 3.3-rc?

> +	struct dev_pm_qos_request pm_qos;
>  
>  	/* Callbacks for clock / power control */
>  	void (*set_pwr)(struct platform_device *host, int state);
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 0e72f1a..576baf5 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -40,6 +40,7 @@
>  #include <linux/module.h>
>  #include <linux/pagemap.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/scatterlist.h>
>  #include <linux/spinlock.h>
> @@ -853,11 +854,27 @@ static int tmio_mmc_get_cd(struct mmc_host *mmc)
>  		return pdata->get_cd(host->pdev);
>  }
>  
> +static int tmio_mmc_enable(struct mmc_host *mmc)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	dev_pm_qos_add_request(mmc->parent, &host->pm_qos, 100);

Why does it add the request for the parent?

> +	return 0;
> +}
> +
> +static int tmio_mmc_disable(struct mmc_host *mmc, int lazy)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	dev_pm_qos_remove_request(&host->pm_qos);
> +	return 0;
> +}
> +
>  static const struct mmc_host_ops tmio_mmc_ops = {
>  	.request	= tmio_mmc_request,
>  	.set_ios	= tmio_mmc_set_ios,
>  	.get_ro         = tmio_mmc_get_ro,
>  	.get_cd		= tmio_mmc_get_cd,
> +	.enable		= tmio_mmc_enable,
> +	.disable	= tmio_mmc_disable,
>  	.enable_sdio_irq = tmio_mmc_enable_sdio_irq,
>  };
>  
> @@ -879,6 +896,8 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
>  	if (!mmc)
>  		return -ENOMEM;
>  
> +	/* disable delayed clock gating, we use PM QoS for precise PM */
> +	mmc->clkgate_delay = 0;

I don't think this is sufficient, because user space can change that value at
any time.  You'd need a mechanism to completely disable the delay regardless
of the user space setting.

>  	pdata->dev = &pdev->dev;
>  	_host = mmc_priv(mmc);
>  	_host->pdata = pdata;
> @@ -899,7 +918,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
>  	}
>  
>  	mmc->ops = &tmio_mmc_ops;
> -	mmc->caps = MMC_CAP_4_BIT_DATA | pdata->capabilities;
> +	mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_DISABLE | pdata->capabilities;
>  	if (pdata->get_clk_rate)
>  		mmc->f_max = pdata->get_clk_rate(pdev);
>  	else

Thanks,
Rafael

Re: [PATCH 2/2] mmc: sh_mmcif: use PM QoS instead of delayed clock gating for PM

[Rafael J. Wysocki]

Hi,

On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> Using delayed clock gating to prevent too frequent gating of the clock
> is simple and efficient, but too inflexible.

I'd mention that you're working around the change made by commit
597dd9d79cfbbb1636d00a7fd0880355d9b20c41.

> We use PM QoS instead to
> let the runtime PM subsystem decide, which power states can be entered
> at any specific time, depending on the currently active governor.

Do I understand correctly that this is going to work, because
sh_mmcif_set_ios() uses pm_runtime_put() while powering off and
pm_runtime_get_sync() when powering on the device?

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/mmc/host/sh_mmcif.c |   27 ++++++++++++++++++++++++---
>  1 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 497e6f4..5a922be 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -55,6 +55,7 @@
>  #include <linux/mmc/sh_mmcif.h>
>  #include <linux/pagemap.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>  #include <linux/module.h>
> @@ -226,6 +227,7 @@ struct sh_mmcif_host {
>  	size_t blocksize;
>  	int sg_idx;
>  	int sg_blkidx;

The fields above are not present in the current mainline tree I have here.

> +	struct dev_pm_qos_request pm_qos;
>  	bool power;
>  	bool card_present;
>  
> @@ -977,10 +979,26 @@ static int sh_mmcif_get_cd(struct mmc_host *mmc)
>  		return p->get_cd(host->pd);
>  }
>  
> +static int sh_mmcif_enable(struct mmc_host *mmc)
> +{
> +	struct sh_mmcif_host *host = mmc_priv(mmc);
> +	dev_pm_qos_add_request(mmc->parent, &host->pm_qos, 100);

Why is the request added for the parent?

> +	return 0;
> +}
> +
> +static int sh_mmcif_disable(struct mmc_host *mmc, int lazy)
> +{
> +	struct sh_mmcif_host *host = mmc_priv(mmc);
> +	dev_pm_qos_remove_request(&host->pm_qos);
> +	return 0;
> +}
> +
>  static struct mmc_host_ops sh_mmcif_ops = {
>  	.request	= sh_mmcif_request,
>  	.set_ios	= sh_mmcif_set_ios,
>  	.get_cd		= sh_mmcif_get_cd,
> +	.enable		= sh_mmcif_enable,
> +	.disable	= sh_mmcif_disable,
>  };
>  
>  static bool sh_mmcif_end_cmd(struct sh_mmcif_host *host)
> @@ -1061,7 +1079,6 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>  {
>  	struct sh_mmcif_host *host = dev_id;
>  	struct mmc_request *mrq = host->mrq;
> -	struct mmc_data *data = mrq->data;

I'm not sure why this is necessary?

>  	cancel_delayed_work_sync(&host->timeout_work);
>  
> @@ -1109,13 +1126,15 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>  	case MMCIF_WAIT_FOR_READ_END:
>  	case MMCIF_WAIT_FOR_WRITE_END:
>  		if (host->sd_error)
> -			data->error = sh_mmcif_error_manage(host);
> +			mrq->data->error = sh_mmcif_error_manage(host);
>  		break;
>  	default:
>  		BUG();
>  	}
>  
>  	if (host->wait_for != MMCIF_WAIT_FOR_STOP) {
> +		struct mmc_data *data = mrq->data;
> +
>  		if (!mrq->cmd->error && data && !data->error)
>  			data->bytes_xfered >  				data->blocks * data->blksz;
> @@ -1303,12 +1322,14 @@ static int __devinit sh_mmcif_probe(struct platform_device *pdev)
>  		mmc->f_min = mmc->f_max / 512;
>  	if (pd->ocr)
>  		mmc->ocr_avail = pd->ocr;
> -	mmc->caps = MMC_CAP_MMC_HIGHSPEED | pd->caps;
> +	mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_DISABLE | pd->caps;
>  	mmc->max_segs = 32;
>  	mmc->max_blk_size = 512;
>  	mmc->max_req_size = PAGE_CACHE_SIZE * mmc->max_segs;
>  	mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
>  	mmc->max_seg_size = mmc->max_req_size;
> +	/* disable delayed clock gating, we use PM QoS for precise PM */
> +	mmc->clkgate_delay = 0;

This isn't sufficient, because clkgate_delay may be changed by user space
at any time later.

>  	sh_mmcif_sync_reset(host);
>  	platform_set_drvdata(pdev, host);
> 

Thanks,
Rafael

Re: [PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Guennadi Liakhovetski]

On Sat, 28 Jan 2012, Rafael J. Wysocki wrote:

> Hi,
> 
> On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> > Hi all
> > 
> > I've already sent one of these two patches yesterday, but I forgot to 
> > include Rafael on the CC list and forgot to mention, that the patch should 
> > be treated as a regression fix and should go into 3.3. The reason why the 
> > commit
> > 
> > commit 7e09bedba1b87f9c7b34ba895b57baf0c36ccdc8
> 
> I think that should be 597dd9d79cfbbb1636d00a7fd0880355d9b20c41, right?
> 
> > Author: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > Date:   Mon Nov 14 13:53:29 2011 +0530
> > 
> >     mmc: core: Use delayed work in clock gating framework
> > 
> > can be considered a regression is, that it extends the powered-on time of 
> > the controller(s) and, possibly, of respective power domains by 200ms (by 
> > default), which is an essential change in behaviour in the negative 
> > direction.
> > 
> > However, if this is indeed the desired change, maybe the better approach 
> > now would be to modify this behaviour in individual drivers, which is 
> > exactly what the two patches in this series are doing.
> 
> I agree that doing such things at the framework level is not a good idea,
> which this example clearly shows.
> 
> The commit above had made the assumption that every platform would
> regard performance as more important than power saving and made a wholesale
> change that affected everyone.  In some cases, however, the change is not
> welcome and is not too easy to work around (because of the user space interface
> file added in the process).
> 
> Quite frankly, I think it would be better to simply revert that commit (along
> with the later one fixing it) at this point, because there may be more cases
> where it actually makes things worse.

Well, it's not my call to decide upon this, I don't know what the global 
MMC preference is, Chris?

> As for the patches, I'll reply to each of them separately.

Right, thanks for the review. Basically your comments are the same for 
both:

1. you'd like not just to change the default initial delay to 0, but to 
prohibit changing it from the userspace completely. For that we need 
Chris' opinion and that cannot be done in MMC host drivers so far, AFAICS. 
Actually, I'm not even sure I agree on this: if the user knows about that 
possibility and consciously adjusts the timeout - why should we prevent 
them from doing so? So far we don't have any user-space interface to 
change device PM QoS parameters, right? So, this gives at least some 
control to the user.

2. you suggest to separate delay=0 and PM QoS. Can do that of course, but 
I thought they were related and that gave us a chance to push PM QoS up 
sooner, rather than later, as a fix. Without it we'd potentially cause the 
controller and the domain to runtime suspend and resume between all 
transactions?

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

Re: [PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Rafael J. Wysocki]

On Wednesday, February 01, 2012, Guennadi Liakhovetski wrote:
> On Sat, 28 Jan 2012, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> > > Hi all
> > > 
> > > I've already sent one of these two patches yesterday, but I forgot to 
> > > include Rafael on the CC list and forgot to mention, that the patch should 
> > > be treated as a regression fix and should go into 3.3. The reason why the 
> > > commit
> > > 
> > > commit 7e09bedba1b87f9c7b34ba895b57baf0c36ccdc8
> > 
> > I think that should be 597dd9d79cfbbb1636d00a7fd0880355d9b20c41, right?
> > 
> > > Author: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > > Date:   Mon Nov 14 13:53:29 2011 +0530
> > > 
> > >     mmc: core: Use delayed work in clock gating framework
> > > 
> > > can be considered a regression is, that it extends the powered-on time of 
> > > the controller(s) and, possibly, of respective power domains by 200ms (by 
> > > default), which is an essential change in behaviour in the negative 
> > > direction.
> > > 
> > > However, if this is indeed the desired change, maybe the better approach 
> > > now would be to modify this behaviour in individual drivers, which is 
> > > exactly what the two patches in this series are doing.
> > 
> > I agree that doing such things at the framework level is not a good idea,
> > which this example clearly shows.
> > 
> > The commit above had made the assumption that every platform would
> > regard performance as more important than power saving and made a wholesale
> > change that affected everyone.  In some cases, however, the change is not
> > welcome and is not too easy to work around (because of the user space interface
> > file added in the process).
> > 
> > Quite frankly, I think it would be better to simply revert that commit (along
> > with the later one fixing it) at this point, because there may be more cases
> > where it actually makes things worse.
> 
> Well, it's not my call to decide upon this, I don't know what the global 
> MMC preference is, Chris?

The question is whether or not everyone (or at least the majority of
people who care) agrees that it's valid to sacrifice power savings for
better performance.

> > As for the patches, I'll reply to each of them separately.
> 
> Right, thanks for the review. Basically your comments are the same for 
> both:
> 
> 1. you'd like not just to change the default initial delay to 0, but to 
> prohibit changing it from the userspace completely.

No, I haven't said that. :-)

I said that setting the delay to 0 initially in the driver wasn't sufficient
to prevent the delay from being used.  If your intention was to use the PM QoS
_along_ with the delay, then you shouldn't advertise that as a regression fix
and I don't see a point setting the delay to 0 initially in the driver in that
case.

> For that we need Chris' opinion and that cannot be done in MMC host drivers
> so far, AFAICS. 
> Actually, I'm not even sure I agree on this: if the user knows about that 
> possibility and consciously adjusts the timeout - why should we prevent 
> them from doing so?

Well, if you want to keep the delay pretty much as is, then there are two
choices that make sense to me: either you change the delay _globally_ to 0,
which kind of reverts the problematic commit, or you keep the default as is
and don't touch it from the driver.  Changing it initially to 0 in the
driver doesn't really help, because it'll requires user space to act
differently depending on what driver is used at the low level (the default will
differ from one driver to another).

> So far we don't have any user-space interface to 
> change device PM QoS parameters, right?

That's because people have been opposing interfeces at the struct device
level and the argument has always been "frameworks will add proper interfaces
for PM QoS", while what's happening here is frameworks adding their own power
management features with questionable interfaces _instead_ _of_ PM QoS.

I thought you wanted to reverse that trend, but apparently I was wrong. :-)

> So, this gives at least some control to the user.

How useful it is is rather unknown at the moment, though.

> 2. you suggest to separate delay=0 and PM QoS. Can do that of course, but 
> I thought they were related and that gave us a chance to push PM QoS up 
> sooner, rather than later, as a fix. Without it we'd potentially cause the 
> controller and the domain to runtime suspend and resume between all 
> transactions?

The problem is that using PM QoS here doesn't fix anything.  Changing the
default delay to 0 kind of does, although I don't like it (for reasons given
above).

I also asked why the PM QoS request was added for the parent of the devices
being handled rather than for the device itself.

Thanks,
Rafael

Re: [PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Guennadi Liakhovetski]

On Wed, 1 Feb 2012, Rafael J. Wysocki wrote:

> On Wednesday, February 01, 2012, Guennadi Liakhovetski wrote:
> > On Sat, 28 Jan 2012, Rafael J. Wysocki wrote:
> > 
> > > Hi,
> > > 
> > > On Thursday, January 19, 2012, Guennadi Liakhovetski wrote:
> > > > Hi all
> > > > 
> > > > I've already sent one of these two patches yesterday, but I forgot to 
> > > > include Rafael on the CC list and forgot to mention, that the patch should 
> > > > be treated as a regression fix and should go into 3.3. The reason why the 
> > > > commit
> > > > 
> > > > commit 7e09bedba1b87f9c7b34ba895b57baf0c36ccdc8
> > > 
> > > I think that should be 597dd9d79cfbbb1636d00a7fd0880355d9b20c41, right?
> > > 
> > > > Author: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > > > Date:   Mon Nov 14 13:53:29 2011 +0530
> > > > 
> > > >     mmc: core: Use delayed work in clock gating framework
> > > > 
> > > > can be considered a regression is, that it extends the powered-on time of 
> > > > the controller(s) and, possibly, of respective power domains by 200ms (by 
> > > > default), which is an essential change in behaviour in the negative 
> > > > direction.
> > > > 
> > > > However, if this is indeed the desired change, maybe the better approach 
> > > > now would be to modify this behaviour in individual drivers, which is 
> > > > exactly what the two patches in this series are doing.
> > > 
> > > I agree that doing such things at the framework level is not a good idea,
> > > which this example clearly shows.
> > > 
> > > The commit above had made the assumption that every platform would
> > > regard performance as more important than power saving and made a wholesale
> > > change that affected everyone.  In some cases, however, the change is not
> > > welcome and is not too easy to work around (because of the user space interface
> > > file added in the process).
> > > 
> > > Quite frankly, I think it would be better to simply revert that commit (along
> > > with the later one fixing it) at this point, because there may be more cases
> > > where it actually makes things worse.
> > 
> > Well, it's not my call to decide upon this, I don't know what the global 
> > MMC preference is, Chris?
> 
> The question is whether or not everyone (or at least the majority of
> people who care) agrees that it's valid to sacrifice power savings for
> better performance.

Are we going to vote? :-)

> > > As for the patches, I'll reply to each of them separately.
> > 
> > Right, thanks for the review. Basically your comments are the same for 
> > both:
> > 
> > 1. you'd like not just to change the default initial delay to 0, but to 
> > prohibit changing it from the userspace completely.
> 
> No, I haven't said that. :-)
> 
> I said that setting the delay to 0 initially in the driver wasn't sufficient
> to prevent the delay from being used.

Yes, sure, I realise that.

> If your intention was to use the PM QoS
> _along_ with the delay, then you shouldn't advertise that as a regression fix
> and I don't see a point setting the delay to 0 initially in the driver in that
> case.

Hmm, I'm not sure to follow here. Firstly, I'm not sure why using both 
cannot classify as a fix, secondly, do you mean, that when using QoS we 
don't have to set the delay to 0? But with the delay left at 200ms PM QoS 
doesn't have much left to say?

> > For that we need Chris' opinion and that cannot be done in MMC host drivers
> > so far, AFAICS. 
> > Actually, I'm not even sure I agree on this: if the user knows about that 
> > possibility and consciously adjusts the timeout - why should we prevent 
> > them from doing so?
> 
> Well, if you want to keep the delay pretty much as is, then there are two
> choices that make sense to me: either you change the delay _globally_ to 0,
> which kind of reverts the problematic commit, or you keep the default as is
> and don't touch it from the driver.  Changing it initially to 0 in the
> driver doesn't really help, because it'll requires user space to act
> differently depending on what driver is used at the low level (the default will
> differ from one driver to another).

Well, following this logic, we should not change the default in the 
driver, and just either ask to revert that commit or rely on user-space to 
set the delay to 0.

> > So far we don't have any user-space interface to 
> > change device PM QoS parameters, right?
> 
> That's because people have been opposing interfeces at the struct device
> level and the argument has always been "frameworks will add proper interfaces
> for PM QoS", while what's happening here is frameworks adding their own power
> management features with questionable interfaces _instead_ _of_ PM QoS.
> 
> I thought you wanted to reverse that trend, but apparently I was wrong. :-)
> 
> > So, this gives at least some control to the user.
> 
> How useful it is is rather unknown at the moment, though.
> 
> > 2. you suggest to separate delay=0 and PM QoS. Can do that of course, but 
> > I thought they were related and that gave us a chance to push PM QoS up 
> > sooner, rather than later, as a fix. Without it we'd potentially cause the 
> > controller and the domain to runtime suspend and resume between all 
> > transactions?
> 
> The problem is that using PM QoS here doesn't fix anything.  Changing the
> default delay to 0 kind of does, although I don't like it (for reasons given
> above).
> 
> I also asked why the PM QoS request was added for the parent of the devices
> being handled rather than for the device itself.

Do you think we should apply constraints to the mmc host class-device?... 
.parent is just a pointer to the actual underlying hardware device, the 
platform device in our case.

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

Re: [PATCH 0/2] mmc: use PM QoS instead of delayed clock gating

[Rafael J. Wysocki]

On Friday, February 03, 2012, Guennadi Liakhovetski wrote:
> On Wed, 1 Feb 2012, Rafael J. Wysocki wrote:
[...]
> > > > As for the patches, I'll reply to each of them separately.
> > > 
> > > Right, thanks for the review. Basically your comments are the same for 
> > > both:
> > > 
> > > 1. you'd like not just to change the default initial delay to 0, but to 
> > > prohibit changing it from the userspace completely.
> > 
> > No, I haven't said that. :-)
> > 
> > I said that setting the delay to 0 initially in the driver wasn't sufficient
> > to prevent the delay from being used.
> 
> Yes, sure, I realise that.
> 
> > If your intention was to use the PM QoS
> > _along_ with the delay, then you shouldn't advertise that as a regression fix
> > and I don't see a point setting the delay to 0 initially in the driver in that
> > case.
> 
> Hmm, I'm not sure to follow here. Firstly, I'm not sure why using both 
> cannot classify as a fix, secondly, do you mean, that when using QoS we 
> don't have to set the delay to 0? But with the delay left at 200ms PM QoS 
> doesn't have much left to say?
> 
> > > For that we need Chris' opinion and that cannot be done in MMC host drivers
> > > so far, AFAICS. 
> > > Actually, I'm not even sure I agree on this: if the user knows about that 
> > > possibility and consciously adjusts the timeout - why should we prevent 
> > > them from doing so?
> > 
> > Well, if you want to keep the delay pretty much as is, then there are two
> > choices that make sense to me: either you change the delay _globally_ to 0,
> > which kind of reverts the problematic commit, or you keep the default as is
> > and don't touch it from the driver.  Changing it initially to 0 in the
> > driver doesn't really help, because it'll requires user space to act
> > differently depending on what driver is used at the low level (the default will
> > differ from one driver to another).
> 
> Well, following this logic, we should not change the default in the 
> driver, and just either ask to revert that commit or rely on user-space to 
> set the delay to 0.

Yes, I meant that.  There's one more option, actually, which is to set the
default to 0 globally, at least for 3.3.  That would make us keep the current
behavior with the possibility to switch to a different delay for whoever wants
that.

> > > So far we don't have any user-space interface to 
> > > change device PM QoS parameters, right?
> > 
> > That's because people have been opposing interfeces at the struct device
> > level and the argument has always been "frameworks will add proper interfaces
> > for PM QoS", while what's happening here is frameworks adding their own power
> > management features with questionable interfaces _instead_ _of_ PM QoS.
> > 
> > I thought you wanted to reverse that trend, but apparently I was wrong. :-)
> > 
> > > So, this gives at least some control to the user.
> > 
> > How useful it is is rather unknown at the moment, though.
> > 
> > > 2. you suggest to separate delay=0 and PM QoS. Can do that of course, but 
> > > I thought they were related and that gave us a chance to push PM QoS up 
> > > sooner, rather than later, as a fix. Without it we'd potentially cause the 
> > > controller and the domain to runtime suspend and resume between all 
> > > transactions?
> > 
> > The problem is that using PM QoS here doesn't fix anything.  Changing the
> > default delay to 0 kind of does, although I don't like it (for reasons given
> > above).
> > 
> > I also asked why the PM QoS request was added for the parent of the devices
> > being handled rather than for the device itself.
> 
> Do you think we should apply constraints to the mmc host class-device?... 
> .parent is just a pointer to the actual underlying hardware device, the 
> platform device in our case.

OK

Thanks,
Rafael