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

[Adrian Hunter <adrian.hunter@nokia.com>]

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.karpov@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.karpov@nokia.com>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@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	500000000	/* 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 is a non-ideal way of doing
things, why inflict it on others.

>> 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:

Yes, sorry about that.  The patch in its present form makes no sense for
the omap tree.  

> 
>    /*
>     * CPUFreq-originated constraint
>     *
>     * In the future, this should be handled by custom OPP clocktype
>     * functions.
>     */
> 
> [...]
> 
>    /**
>     * omap_pm_cpu_set_freq - set the current minimum MPU frequency
>     * @f: MPU frequency in Hz
>     *
>     * Set the current minimum CPU frequency.  The actual CPU frequency
>     * used could end up higher if the DSP requested a higher OPP.
>     * Intended to be called by plat-omap/cpu_omap.c:omap_target().  No
>     * return value.
>     */
>    void omap_pm_cpu_set_freq(unsigned long f);
> 
> To my reading, this file is quite clear that this function is intended to 
> be called only from CPUFreq.
> 
>>> 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.
> 
> omap_pm_set_min_bus_tput() addresses a different case than 
> omap_pm_cpu_set_freq().  It's just intended to ensure that the 
> interconnect clock frequency is high enough to sustain the desired data 
> rate.  It presumably won't affect the performance problem that this patch 
> is intended to address.
> 
> 
> regards,
> 
> - Paul