Re: [PATCH 1/3] OMAP: omap_device: optionally auto-adjust device activate/deactivate latencies

[Nishanth Menon <nm@ti.com>]

Kevin Hilman had written, on 01/11/2010 06:50 PM, the following:
> Nishanth Menon <menon.nishanth@gmail.com> writes:
> 
>> Kevin Hilman said the following on 01/08/2010 05:26 PM:
>>> First, this patch adds new worst-case latency values to the
>>> omap_device_pm_latency struct.  Here the worst-case measured latencies
>>> for the activate and deactivate hooks are stored.
>>>
>>> In addition, add an option to auto-adjust the latency values used for
>>> device activate/deactivate.
>>>
>>> By setting a new 'OMAP_DEVICE_LATENCY_AUTO_ADJUST' flag in the
>>> omap_device_pm_latency struct, the omap_device layer automatically
>>> adjusts the activate/deactivate latencies to the worst-case measured
>>> values.
>>>
>>> Anytime a new worst-case value is found, it is printed to the console.
>>> Here is an example log during boot using UART2 s an example.  After
>>> boot, the OPP is manually changed to the 125MHz OPP:
>>>
>>> [...]
>>> Freeing init memory: 128K
>>> omap_device: serial8250.2: new worst case deactivate latency 0: 30517
>>> omap_device: serial8250.2: new worst case activate latency 0: 30517
>>> omap_device: serial8250.2: new worst case activate latency 0: 218139648
>>> omap_device: serial8250.2: new worst case deactivate latency 0: 61035
>>> omap_device: serial8250.2: new worst case activate latency 0: 278076171
>>> omap_device: serial8250.2: new worst case activate latency 0: 298614501
>>> omap_device: serial8250.2: new worst case activate latency 0: 327331542
>>>
>>> / # echo 125000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
>>>
>>> omap_device: serial8250.2: new worst case deactivate latency 0: 91552
>>>
>>> Motivation: this can be used as a technique to automatically determine
>>> the worst case latency values.  The current method of printing a
>>> warning on every violation is too noisy to actually interact the
>>> console in order to set low OPP to discover latencies.
>>>
>>> Another motivation for this patch is that the activate/deactivate
>>> latenices can vary depending on the idlemode of the device.  While
>>> working on the UARTs, I noticed that when using no-idle, the activate
>>> latencies were as high as several hundred msecs as shown above.  When
>>> the UARTs are in smart-idle, the max latency is well under 100 usecs.
>>>
>>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>>> ---
>>>  arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
>>>  arch/arm/plat-omap/omap_device.c              |   41 ++++++++++++++++++++-----
>>>  2 files changed, 37 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
>>> index dc1fac1..76d4917 100644
>>> --- a/arch/arm/plat-omap/include/plat/omap_device.h
>>> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
>>> @@ -131,11 +131,15 @@ int omap_device_enable_clocks(struct omap_device *od);
>>>   */
>>>  struct omap_device_pm_latency {
>>>  	u32 deactivate_lat;
>>> +	u32 deactivate_lat_worst;
>>>  	int (*deactivate_func)(struct omap_device *od);
>>>  	u32 activate_lat;
>>> +	u32 activate_lat_worst;
>>>  	int (*activate_func)(struct omap_device *od);
>>> +	u32 flags;
>>>  };
>>>  +#define OMAP_DEVICE_LATENCY_AUTO_ADJUST BIT(1)
>>>   /* Get omap_device pointer from platform_device pointer */
>>>  #define to_omap_device(x) container_of((x), struct omap_device, pdev)
>>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>>> index 1e5648d..d8c75c8 100644
>>> --- a/arch/arm/plat-omap/omap_device.c
>>> +++ b/arch/arm/plat-omap/omap_device.c
>>> @@ -148,10 +148,22 @@ static int _omap_device_activate(struct omap_device *od, u8 ignore_lat)
>>>  			 "%llu nsec\n", od->pdev.name, od->pm_lat_level,
>>>  			 act_lat);
>>>  -		WARN(act_lat > odpl->activate_lat, "omap_device:
>>> %s.%d: "
>>> -		     "activate step %d took longer than expected (%llu > %d)\n",
>>> -		     od->pdev.name, od->pdev.id, od->pm_lat_level,
>>> -		     act_lat, odpl->activate_lat);
>>> +		if (act_lat > odpl->activate_lat) {
>>> +			odpl->activate_lat_worst = act_lat;
>>> +			if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
>>> +				odpl->activate_lat = act_lat;
>>> +				pr_warning("omap_device: %s.%d: new worst case "
>>> +					   "activate latency %d: %llu\n",
>>> +					   od->pdev.name, od->pdev.id,
>>> +					   od->pm_lat_level, act_lat);
>>>   
>> nitpicky dumb comment: since the flags say auto adjust, do you care to
>> make this just a pr_info instead of a warning. it is not the same
>> severity as latency>activate_latency without AUTO_ADJUST right?
> 
> Agreed, will change to pr_info()
Thanks.

> 
>>> +			} else
>>> +				pr_warning("omap_device: %s.%d: activate "
>>> +					   "latency %d higher than exptected. "
>>> +					   "(%llu > %d)\n",
>>> +					   od->pdev.name, od->pdev.id,
>>> +					   od->pm_lat_level, act_lat,
>>> +					   odpl->activate_lat);
>>>   
>> nitpick: I think you need {} for the else part too now a days..
> 
> you mean as a CodingStyle issue, or a compiler issue?
> do you have a reference for this requirement? 
> 
> do you mean if the 'if' part has {}, the else part should too, even if
> it's a single line?
> 
> I don't really care one way or the other, just want to know more about
> what you're suggesting.
Apologies on the obscure comment. I meant Coding style.
Documentation/CodingStyle says:

  171 This does not apply if one branch of a conditional statement is a 
single
  172 statement. Use braces in both branches.
  173
  174 if (condition) {
  175         do_this();
  176         do_that();
  177 } else {
  178         otherwise();
  179 }


-- 
Regards,
Nishanth Menon