Re: [PATCH 4/8] omap_hsmmc: set DVFS/PM constraints

2010-01-15 Thread Adrian Hunter 

Paul Walmsley wrote:

Hello Adrian,



Thanks for your comments.  I will be dropping this patch for now.

There are a couple more comments below.


On Thu, 14 Jan 2010, Adrian Hunter wrote:


Paul Walmsley wrote:

(added Denis and Kevin)

Hello Denis, Adrian,

On Wed, 13 Jan 2010, Adrian Hunter wrote:


From afab8b432b37ae1f42b281e58989c8d607ed7183 Mon Sep 17 00:00:00 2001
From: Denis Karpov ext-denis.2.kar...@nokia.com
Date: Wed, 8 Jul 2009 16:15:08 +0200
Subject: [PATCH] omap_hsmmc: set DVFS/PM constraints

Set constraint for MPU minimal frequency to maintain good
I/O performance.

The constraint is set in MMC host 'ENABLED' state and dropped
when MMC host enters 'DISABLED' state which currently happens
upon 100ms of inactivity.

Signed-off-by: Denis Karpov ext-denis.2.kar...@nokia.com
Signed-off-by: Adrian Hunter adrian.hun...@nokia.com
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |   18 ++
 arch/arm/mach-omap2/hsmmc.c  |2 ++
 arch/arm/mach-omap2/hsmmc.h  |2 ++
 arch/arm/plat-omap/include/plat/mmc.h|3 +++
 drivers/mmc/host/omap_hsmmc.c|   17 +
 5 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ab07ca2..b6318b1 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -209,6 +209,22 @@ static struct twl4030_madc_platform_data
rx51_madc_data = {
.irq_line   = 1,
 };
 +#if defined(CONFIG_BRIDGE_DVFS)
+/*
+ * This handler can be used for setting other DVFS/PM constraints:
+ * intr latency, wakeup latency, DMA start latency, bus throughput
+ * according to API in mach/omap-pm.h.  Currently we only set constraints
+ * for MPU frequency.
+ */
+#define MMC_MIN_MPU_FREQUENCY  5   /* S500M at OPP3 */
+static void rx51_mmc_set_pm_constraints(struct device *dev, int on)
+{
+   omap_pm_set_min_mpu_freq(dev, (on ? MMC_MIN_MPU_FREQUENCY : 0));
+}

NAK.  The MMC driver (or any other driver, for that matter) must not set MPU
frequency constraints merely to boost performance.  Drivers have no way of
knowing what the power vs. performance policy is for a given device or use
case.

The driver doesn't but RX-51 does, which is why the code is in the RX-51
board file.  


I don't think that changes the situation.  The RX-51 board file represents 
the hardware integration on the device, not MPU power policy.  That's the 
CPUFreq governor's responsibility.  It shouldn't be necessary to hack a 
board file to change the CPU power management policy.  Maybe that is 
acceptable on a device that has been locked down by the manufacturer to 
only boot kernels signed by them, but that's not the case for RX-51.


I agree it is not the ideal way to do it.  I will drop this patch while
we try to find a better solution.

Denis didn't go into detail on the performance problem that you and he 
observed.  Further info would be welcome.  Hypothesizing, if the problem 
is that MMC does a lot of MPU work before the CPUFreq timer fires to 
re-evaluate performance, then maybe some CPUFreq function call needs to 
exist to ask the CPUFreq governor to elevate MPU speed in advance.  But 
it's hard to say without knowing more about the problem you observed.


We were not able to identify a single source of the reduced performance.
We suspect it is a combination of factors, all of which are addressed
by operating at a faster operating point.




We know exactly the use cases and the effect on performance.


Certainly for Maemo 5 Harmattan as shipped that is true.  It's not 
necessarily true if someone wants to boot another distribution like Debian 
and use (for example) a userspace CPUFreq governor on the device.


I think we know the 3 people in the world that might try that and they
can make their own kernel ;-)

Seriously though, I would argue that end users would prefer good MMC
performance.


If the system is not upscaling MPU frequency quickly enough, then the power
management policy code -- CPUFreq, in the MPU case -- or the parameters for
that code -- need to be adjusted.  (Of course, integrators can always dump
hacks like this in their own trees if they get lazy, but these should be
kept far, far away from mainline.)

It is unreasonable to override the policy decisions of the board maker
as defined in their board files.


Could you explain why?  MPU power management policy for an on-chip device 
such as MMC, which is located internally in the OMAP SoC, is board 
hardware-invariant, and so doesn't belong in the board file.  I agree that 
this is dependent on the software distribution.  So we either need to 
understand the problem and come up with a clean way to resolve it, or keep 
hacks like this distribution-specific.


I assumed that other boards could have completely different use-cases and
completely different operating points.  Since it

Re: [PATCH 4/8] omap_hsmmc: set DVFS/PM constraints

2010-01-14 Thread Adrian Hunter 

Paul Walmsley wrote:

(added Denis and Kevin)

Hello Denis, Adrian,

On Wed, 13 Jan 2010, Adrian Hunter wrote:


From afab8b432b37ae1f42b281e58989c8d607ed7183 Mon Sep 17 00:00:00 2001
From: Denis Karpov ext-denis.2.kar...@nokia.com
Date: Wed, 8 Jul 2009 16:15:08 +0200
Subject: [PATCH] omap_hsmmc: set DVFS/PM constraints

Set constraint for MPU minimal frequency to maintain good
I/O performance.

The constraint is set in MMC host 'ENABLED' state and dropped
when MMC host enters 'DISABLED' state which currently happens
upon 100ms of inactivity.

Signed-off-by: Denis Karpov ext-denis.2.kar...@nokia.com
Signed-off-by: Adrian Hunter adrian.hun...@nokia.com
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |   18 ++
 arch/arm/mach-omap2/hsmmc.c  |2 ++
 arch/arm/mach-omap2/hsmmc.h  |2 ++
 arch/arm/plat-omap/include/plat/mmc.h|3 +++
 drivers/mmc/host/omap_hsmmc.c|   17 +
 5 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c 
b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ab07ca2..b6318b1 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -209,6 +209,22 @@ static struct twl4030_madc_platform_data rx51_madc_data = {
.irq_line   = 1,
 };
 
+#if defined(CONFIG_BRIDGE_DVFS)

+/*
+ * This handler can be used for setting other DVFS/PM constraints:
+ * intr latency, wakeup latency, DMA start latency, bus throughput
+ * according to API in mach/omap-pm.h.  Currently we only set constraints
+ * for MPU frequency.
+ */
+#define MMC_MIN_MPU_FREQUENCY  5   /* S500M at OPP3 */
+static void rx51_mmc_set_pm_constraints(struct device *dev, int on)
+{
+   omap_pm_set_min_mpu_freq(dev, (on ? MMC_MIN_MPU_FREQUENCY : 0));
+}


NAK.  The MMC driver (or any other driver, for that matter) must not set 
MPU frequency constraints merely to boost performance.  Drivers have no 
way of knowing what the power vs. performance policy is for a given device 
or use case.


The driver doesn't but RX-51 does, which is why the code is in the RX-51
board file.  We know exactly the use cases and the effect on performance.

If the system is not upscaling MPU frequency quickly enough, then the 
power management policy code -- CPUFreq, in the MPU case -- or the 
parameters for that code -- need to be adjusted.  (Of course, integrators 
can always dump hacks like this in their own trees if they get lazy, but 
these should be kept far, far away from mainline.)


It is unreasonable to override the policy decisions of the board maker
as defined in their board files.

Instead you must remove the omap_pm_set_min_mpu_freq() function entirely
or suffer the consequence that it can be used. i.e. it should not exist
if you don't want anyone to use it.



N.B.  As a separate matter, the MMC driver should call 
omap_pm_set_min_bus_tput() with the maximum throughput that the current 
MMC card can sustain to memory.  A reasonable upper bound should be easy 
to calculate based on the MMC clock speed and the width of the MMC 
transfers.  This will allow the kernel to adjust the bus frequency 
appropriately as the OMAP PM core's bus utilization model improves.


Many different constraints were tried.  min_mpu_freq was the only one that
worked.

In the future, improvements to DMA and changes to PM may be able to get the
same performance without the min_mpu_freq constraint.




- Paul



--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] omap_hsmmc: set DVFS/PM constraints

2010-01-14 Thread Paul Walmsley 
Hello Adrian,

On Thu, 14 Jan 2010, Adrian Hunter wrote:

 Paul Walmsley wrote:
  (added Denis and Kevin)
  
  Hello Denis, Adrian,
  
  On Wed, 13 Jan 2010, Adrian Hunter wrote:
  
   From afab8b432b37ae1f42b281e58989c8d607ed7183 Mon Sep 17 00:00:00 2001
   From: Denis Karpov ext-denis.2.kar...@nokia.com
   Date: Wed, 8 Jul 2009 16:15:08 +0200
   Subject: [PATCH] omap_hsmmc: set DVFS/PM constraints
   
   Set constraint for MPU minimal frequency to maintain good
   I/O performance.
   
   The constraint is set in MMC host 'ENABLED' state and dropped
   when MMC host enters 'DISABLED' state which currently happens
   upon 100ms of inactivity.
   
   Signed-off-by: Denis Karpov ext-denis.2.kar...@nokia.com
   Signed-off-by: Adrian Hunter adrian.hun...@nokia.com
   ---
arch/arm/mach-omap2/board-rx51-peripherals.c |   18 ++
arch/arm/mach-omap2/hsmmc.c  |2 ++
arch/arm/mach-omap2/hsmmc.h  |2 ++
arch/arm/plat-omap/include/plat/mmc.h|3 +++
drivers/mmc/host/omap_hsmmc.c|   17 +
5 files changed, 42 insertions(+), 0 deletions(-)
   
   diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
   b/arch/arm/mach-omap2/board-rx51-peripherals.c
   index ab07ca2..b6318b1 100644
   --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
   +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
   @@ -209,6 +209,22 @@ static struct twl4030_madc_platform_data
   rx51_madc_data = {
 .irq_line   = 1,
};
+#if defined(CONFIG_BRIDGE_DVFS)
   +/*
   + * This handler can be used for setting other DVFS/PM constraints:
   + * intr latency, wakeup latency, DMA start latency, bus throughput
   + * according to API in mach/omap-pm.h.  Currently we only set constraints
   + * for MPU frequency.
   + */
   +#define MMC_MIN_MPU_FREQUENCY5   /* S500M at OPP3 */
   +static void rx51_mmc_set_pm_constraints(struct device *dev, int on)
   +{
   + omap_pm_set_min_mpu_freq(dev, (on ? MMC_MIN_MPU_FREQUENCY : 0));
   +}
  
  NAK.  The MMC driver (or any other driver, for that matter) must not set MPU
  frequency constraints merely to boost performance.  Drivers have no way of
  knowing what the power vs. performance policy is for a given device or use
  case.
 
 The driver doesn't but RX-51 does, which is why the code is in the RX-51
 board file.  

I don't think that changes the situation.  The RX-51 board file represents 
the hardware integration on the device, not MPU power policy.  That's the 
CPUFreq governor's responsibility.  It shouldn't be necessary to hack a 
board file to change the CPU power management policy.  Maybe that is 
acceptable on a device that has been locked down by the manufacturer to 
only boot kernels signed by them, but that's not the case for RX-51.

Denis didn't go into detail on the performance problem that you and he 
observed.  Further info would be welcome.  Hypothesizing, if the problem 
is that MMC does a lot of MPU work before the CPUFreq timer fires to 
re-evaluate performance, then maybe some CPUFreq function call needs to 
exist to ask the CPUFreq governor to elevate MPU speed in advance.  But 
it's hard to say without knowing more about the problem you observed.

 We know exactly the use cases and the effect on performance.

Certainly for Maemo 5 Harmattan as shipped that is true.  It's not 
necessarily true if someone wants to boot another distribution like Debian 
and use (for example) a userspace CPUFreq governor on the device.

  If the system is not upscaling MPU frequency quickly enough, then the power
  management policy code -- CPUFreq, in the MPU case -- or the parameters for
  that code -- need to be adjusted.  (Of course, integrators can always dump
  hacks like this in their own trees if they get lazy, but these should be
  kept far, far away from mainline.)
 
 It is unreasonable to override the policy decisions of the board maker
 as defined in their board files.

Could you explain why?  MPU power management policy for an on-chip device 
such as MMC, which is located internally in the OMAP SoC, is board 
hardware-invariant, and so doesn't belong in the board file.  I agree that 
this is dependent on the software distribution.  So we either need to 
understand the problem and come up with a clean way to resolve it, or keep 
hacks like this distribution-specific.

 Instead you must remove the omap_pm_set_min_mpu_freq() function entirely
 or suffer the consequence that it can be used. i.e. it should not exist
 if you don't want anyone to use it.

This function, omap_pm_set_min_mpu_freq(), does not even exist in 
mainline, Tony's kernel, or the current PM branch.  Nor does 
CONFIG_BRIDGE_DVFS.  When you look at the file that defines the interface 
for these functions, arch/arm/plat-omap/include/plat/omap-pm.h, you'll 
find this:

   /*
* CPUFreq-originated constraint
*
* In the future, this should be handled by custom OPP clocktype

[PATCH 4/8] omap_hsmmc: set DVFS/PM constraints

2010-01-13 Thread Adrian Hunter 
From afab8b432b37ae1f42b281e58989c8d607ed7183 Mon Sep 17 00:00:00 2001
From: Denis Karpov ext-denis.2.kar...@nokia.com
Date: Wed, 8 Jul 2009 16:15:08 +0200
Subject: [PATCH] omap_hsmmc: set DVFS/PM constraints

Set constraint for MPU minimal frequency to maintain good
I/O performance.

The constraint is set in MMC host 'ENABLED' state and dropped
when MMC host enters 'DISABLED' state which currently happens
upon 100ms of inactivity.

Signed-off-by: Denis Karpov ext-denis.2.kar...@nokia.com
Signed-off-by: Adrian Hunter adrian.hun...@nokia.com
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |   18 ++
 arch/arm/mach-omap2/hsmmc.c  |2 ++
 arch/arm/mach-omap2/hsmmc.h  |2 ++
 arch/arm/plat-omap/include/plat/mmc.h|3 +++
 drivers/mmc/host/omap_hsmmc.c|   17 +
 5 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c 
b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ab07ca2..b6318b1 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -209,6 +209,22 @@ static struct twl4030_madc_platform_data rx51_madc_data = {
.irq_line   = 1,
 };
 
+#if defined(CONFIG_BRIDGE_DVFS)
+/*
+ * This handler can be used for setting other DVFS/PM constraints:
+ * intr latency, wakeup latency, DMA start latency, bus throughput
+ * according to API in mach/omap-pm.h.  Currently we only set constraints
+ * for MPU frequency.
+ */
+#define MMC_MIN_MPU_FREQUENCY  5   /* S500M at OPP3 */
+static void rx51_mmc_set_pm_constraints(struct device *dev, int on)
+{
+   omap_pm_set_min_mpu_freq(dev, (on ? MMC_MIN_MPU_FREQUENCY : 0));
+}
+#else
+#define rx51_mmc_set_pm_constraints NULL
+#endif
+
 static struct omap2_hsmmc_info mmc[] __initdata = {
{
.name   = external,
@@ -218,6 +234,7 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
.gpio_cd= 160,
.gpio_wp= -EINVAL,
.power_saving   = true,
+   .set_pm_constraints = rx51_mmc_set_pm_constraints,
},
{
.name   = internal,
@@ -227,6 +244,7 @@ static struct omap2_hsmmc_info mmc[] __initdata = {
.gpio_wp= -EINVAL,
.nonremovable   = true,
.power_saving   = true,
+   .set_pm_constraints = rx51_mmc_set_pm_constraints,
},
{}  /* Terminator */
 };
diff --git a/arch/arm/mach-omap2/hsmmc.c b/arch/arm/mach-omap2/hsmmc.c
index 76a1f6c..1211d15 100644
--- a/arch/arm/mach-omap2/hsmmc.c
+++ b/arch/arm/mach-omap2/hsmmc.c
@@ -76,6 +76,8 @@ void __init omap2_hsmmc_init(struct omap2_hsmmc_info 
*controllers)
 
mmc-get_context_loss_count = hsmmc_get_context_loss;
 
+   mmc-set_pm_constraints = c-set_pm_constraints;
+
mmc-slots[0].switch_pin = c-gpio_cd;
mmc-slots[0].gpio_wp = c-gpio_wp;
 
diff --git a/arch/arm/mach-omap2/hsmmc.h b/arch/arm/mach-omap2/hsmmc.h
index e946b5f..ad437f3 100644
--- a/arch/arm/mach-omap2/hsmmc.h
+++ b/arch/arm/mach-omap2/hsmmc.h
@@ -19,6 +19,8 @@ struct omap2_hsmmc_info {
char*name;  /* or NULL for default */
struct device *dev; /* returned: pointer to mmc adapter */
int ocr_mask;   /* temporary HACK */
+   /* Set/drop DVFS/PM constraints */
+   void (*set_pm_constraints)(struct device *dev, int on);
 };
 
 #if defined(CONFIG_MMC_OMAP_HS) || defined(CONFIG_MMC_OMAP_HS_MODULE)
diff --git a/arch/arm/plat-omap/include/plat/mmc.h 
b/arch/arm/plat-omap/include/plat/mmc.h
index 6af87b0..c3e0c34 100644
--- a/arch/arm/plat-omap/include/plat/mmc.h
+++ b/arch/arm/plat-omap/include/plat/mmc.h
@@ -69,6 +69,9 @@ struct omap_mmc_platform_data {
/* Return context loss count due to PM states changing */
int (*get_context_loss_count)(struct device *dev);
 
+   /* Set/drop DVFS/PM constraints */
+   void (*set_pm_constraints)(struct device *dev, int on);
+
u64 dma_mask;
 
struct omap_mmc_slot_data {
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 63d24df..e2f63a5 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1677,7 +1677,13 @@ enum {ENABLED = 0, DISABLED, CARDSLEEP, REGSLEEP, OFF};
 static int omap_hsmmc_enabled_to_disabled(struct omap_hsmmc_host *host)
 {
omap_hsmmc_context_save(host);
+
clk_disable(host-fclk);
+
+   /* Drop PM/DVFS constraints */
+   if (host-pdata-set_pm_constraints)
+   host-pdata-set_pm_constraints(host-dev, 0);
+
host-dpm_state = DISABLED;
 
dev_dbg(mmc_dev(host-mmc), ENABLED - DISABLED\n);
@@ -1768,6 +1774,10 @@ static int omap_hsmmc_disabled_to_enabled(struct 
omap_hsmmc_host *host)
return err;

Re: [PATCH 4/8] omap_hsmmc: set DVFS/PM constraints

2010-01-13 Thread Paul Walmsley 
(added Denis and Kevin)

Hello Denis, Adrian,

On Wed, 13 Jan 2010, Adrian Hunter wrote:

 From afab8b432b37ae1f42b281e58989c8d607ed7183 Mon Sep 17 00:00:00 2001
 From: Denis Karpov ext-denis.2.kar...@nokia.com
 Date: Wed, 8 Jul 2009 16:15:08 +0200
 Subject: [PATCH] omap_hsmmc: set DVFS/PM constraints
 
 Set constraint for MPU minimal frequency to maintain good
 I/O performance.
 
 The constraint is set in MMC host 'ENABLED' state and dropped
 when MMC host enters 'DISABLED' state which currently happens
 upon 100ms of inactivity.
 
 Signed-off-by: Denis Karpov ext-denis.2.kar...@nokia.com
 Signed-off-by: Adrian Hunter adrian.hun...@nokia.com
 ---
  arch/arm/mach-omap2/board-rx51-peripherals.c |   18 ++
  arch/arm/mach-omap2/hsmmc.c  |2 ++
  arch/arm/mach-omap2/hsmmc.h  |2 ++
  arch/arm/plat-omap/include/plat/mmc.h|3 +++
  drivers/mmc/host/omap_hsmmc.c|   17 +
  5 files changed, 42 insertions(+), 0 deletions(-)
 
 diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c 
 b/arch/arm/mach-omap2/board-rx51-peripherals.c
 index ab07ca2..b6318b1 100644
 --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
 +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
 @@ -209,6 +209,22 @@ static struct twl4030_madc_platform_data rx51_madc_data 
 = {
   .irq_line   = 1,
  };
  
 +#if defined(CONFIG_BRIDGE_DVFS)
 +/*
 + * This handler can be used for setting other DVFS/PM constraints:
 + * intr latency, wakeup latency, DMA start latency, bus throughput
 + * according to API in mach/omap-pm.h.  Currently we only set constraints
 + * for MPU frequency.
 + */
 +#define MMC_MIN_MPU_FREQUENCY5   /* S500M at OPP3 */
 +static void rx51_mmc_set_pm_constraints(struct device *dev, int on)
 +{
 + omap_pm_set_min_mpu_freq(dev, (on ? MMC_MIN_MPU_FREQUENCY : 0));
 +}

NAK.  The MMC driver (or any other driver, for that matter) must not set 
MPU frequency constraints merely to boost performance.  Drivers have no 
way of knowing what the power vs. performance policy is for a given device 
or use case.

If the system is not upscaling MPU frequency quickly enough, then the 
power management policy code -- CPUFreq, in the MPU case -- or the 
parameters for that code -- need to be adjusted.  (Of course, integrators 
can always dump hacks like this in their own trees if they get lazy, but 
these should be kept far, far away from mainline.)

N.B.  As a separate matter, the MMC driver should call 
omap_pm_set_min_bus_tput() with the maximum throughput that the current 
MMC card can sustain to memory.  A reasonable upper bound should be easy 
to calculate based on the MMC clock speed and the width of the MMC 
transfers.  This will allow the kernel to adjust the bus frequency 
appropriately as the OMAP PM core's bus utilization model improves.


- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html