[PATCH 0/3] misc. omap_device/omap_hwmod updates

[PATCH 0/3] misc. omap_device/omap_hwmod updates

[Kevin Hilman]

A small series of proposed updates for omap_device/omap_hwmod.

This series was tested on the current PM branch but also applies cleanly to current linux-omap master branch.  

It was tested along with previously posted changes to convert UARTs to
omap_device.  A new version of the UART omap_device conversion will be
arriving soon-ish.

Kevin

Kevin Hilman (3):
  OMAP: omap_device: optionally auto-adjust device activate/deactivate
    latencies
  OMAP: hwmod: add read/write API for SYSCONFIG
  OMAP: hwmod: allow idle after HWMOD_INIT_NO_IDLE

 arch/arm/mach-omap2/omap_hwmod.c              |   21 ++++++++++++-
 arch/arm/plat-omap/include/plat/omap_device.h |    4 ++
 arch/arm/plat-omap/include/plat/omap_hwmod.h  |    3 ++
 arch/arm/plat-omap/omap_device.c              |   41 ++++++++++++++++++++-----
 4 files changed, 60 insertions(+), 9 deletions(-)

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

[Kevin Hilman]

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);
+			} 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);
+		}
 
 		od->dev_wakeup_lat -= odpl->activate_lat;
 	}
@@ -204,10 +216,23 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)
 			 "%llu nsec\n", od->pdev.name, od->pm_lat_level,
 			 deact_lat);
 
-		WARN(deact_lat > odpl->deactivate_lat, "omap_device: %s.%d: "
-		     "deactivate step %d took longer than expected "
-		     "(%llu > %d)\n", od->pdev.name, od->pdev.id,
-		     od->pm_lat_level, deact_lat, odpl->deactivate_lat);
+		if (deact_lat > odpl->deactivate_lat) {
+			odpl->deactivate_lat_worst = deact_lat;
+			if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
+				odpl->deactivate_lat = deact_lat;
+				pr_warning("omap_device: %s.%d: new worst case "
+					   "deactivate latency %d: %llu\n",
+					   od->pdev.name, od->pdev.id,
+					   od->pm_lat_level, deact_lat);
+			} else
+				pr_warning("omap_device: %s.%d: deactivate "
+					   "latency %d higher than exptected. "
+					   "(%llu > %d)\n",
+					   od->pdev.name, od->pdev.id,
+					   od->pm_lat_level, deact_lat,
+					   odpl->deactivate_lat);
+		}
+
 
 		od->dev_wakeup_lat += odpl->activate_lat;
 
-- 
1.6.6.rc2.1.g42108

[PATCH 2/3] OMAP: hwmod: add read/write API for SYSCONFIG

[Kevin Hilman]

Some HW blocks have errata which requires selective enabling/disabling
of SYSCONFIG bits.  In particular, some blocks have known issues with
smart-idle so smart-idle has to be disabled under certain conditions.

Add API to read/write a modules SYSCONFIG register which takes
advantage of the built-in caching of omap_hwmod.  Any manual
reading/writing of SYSCONFIG registers outside of hwmod will cause
potential problems in omap_hwmod due to the caching.

RFC: would an API to only touch smart-idle be more appropriate?
Maybe omap_hwmod_smart_idle_enable(oh, bool enable)?

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   10 ++++++++++
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    3 +++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index d8c8545..307deea 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -993,6 +993,16 @@ void omap_hwmod_writel(u32 v, struct omap_hwmod *oh, u16 reg_offs)
 	__raw_writel(v, oh->_rt_va + reg_offs);
 }
 
+u32 omap_hwmod_read_sysc(struct omap_hwmod *oh)
+{
+	return oh->_sysc_cache;
+}
+
+void omap_hwmod_write_sysc(u32 v, struct omap_hwmod *oh)
+{
+	_write_sysconfig(v, oh);
+}
+
 /**
  * omap_hwmod_register - register a struct omap_hwmod
  * @oh: struct omap_hwmod *
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 007935a..db1e6ef 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -446,6 +446,9 @@ void omap_hwmod_ocp_barrier(struct omap_hwmod *oh);
 void omap_hwmod_writel(u32 v, struct omap_hwmod *oh, u16 reg_offs);
 u32 omap_hwmod_readl(struct omap_hwmod *oh, u16 reg_offs);
 
+void omap_hwmod_write_sysc(u32 v, struct omap_hwmod *oh);
+u32 omap_hwmod_read_sysc(struct omap_hwmod *oh);
+
 int omap_hwmod_count_resources(struct omap_hwmod *oh);
 int omap_hwmod_fill_resources(struct omap_hwmod *oh, struct resource *res);
 
-- 
1.6.6.rc2.1.g42108

[PATCH 3/3] OMAP: hwmod: allow idle after HWMOD_INIT_NO_IDLE

[Kevin Hilman]

If an omap_hwmod is setup using HWMOD_INIT_NO_IDLE flag, there is
currently way to idle it since omap_hwmod_idle() requires the hwmod to
be in the enabled state.

This patch adds a check to omap_hwmod_idle() so if the hwmod was has
the INIT_NO_IDLE flag, calling omap_hwmod_idle() will still succeed.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 307deea..2de4cc3 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -869,7 +869,13 @@ static int _enable(struct omap_hwmod *oh)
  */
 static int _idle(struct omap_hwmod *oh)
 {
-	if (oh->_state != _HWMOD_STATE_ENABLED) {
+	/*
+	 * To idle, hwmod must be enabled, EXCEPT if hwmod was
+	 * initialized using the INIT_NO_IDLE flag.  In this case it
+	 * will not yet be enabled so we have to allow it to be idled.
+	 */
+	if ((oh->_state != _HWMOD_STATE_ENABLED) &&
+	    !(oh->flags & HWMOD_INIT_NO_IDLE)) {
 		WARN(1, "omap_hwmod: %s: idle state can only be entered from "
 		     "enabled state\n", oh->name);
 		return -EINVAL;
@@ -884,6 +890,9 @@ static int _idle(struct omap_hwmod *oh)
 
 	oh->_state = _HWMOD_STATE_IDLE;
 
+	/* Clear init flag which should only affect first call to idle */
+	oh->flags &= ~HWMOD_INIT_NO_IDLE;
+
 	return 0;
 }
 
-- 
1.6.6.rc2.1.g42108

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

[Nishanth Menon]

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?
> +			} 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..

> +		}
>  
>  		od->dev_wakeup_lat -= odpl->activate_lat;
>  	}
> @@ -204,10 +216,23 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)
>  			 "%llu nsec\n", od->pdev.name, od->pm_lat_level,
>  			 deact_lat);
>  
> -		WARN(deact_lat > odpl->deactivate_lat, "omap_device: %s.%d: "
> -		     "deactivate step %d took longer than expected "
> -		     "(%llu > %d)\n", od->pdev.name, od->pdev.id,
> -		     od->pm_lat_level, deact_lat, odpl->deactivate_lat);
> +		if (deact_lat > odpl->deactivate_lat) {
> +			odpl->deactivate_lat_worst = deact_lat;
> +			if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
> +				odpl->deactivate_lat = deact_lat;
> +				pr_warning("omap_device: %s.%d: new worst case "
> +					   "deactivate latency %d: %llu\n",
> +					   od->pdev.name, od->pdev.id,
> +					   od->pm_lat_level, deact_lat);
>   
same as previous.
> +			} else
> +				pr_warning("omap_device: %s.%d: deactivate "
> +					   "latency %d higher than exptected. "
> +					   "(%llu > %d)\n",
> +					   od->pdev.name, od->pdev.id,
> +					   od->pm_lat_level, deact_lat,
> +					   odpl->deactivate_lat);
>   
same here.
> +		}
> +
>  
>  		od->dev_wakeup_lat += odpl->activate_lat;
>  
>   
Regards,
Nishanth Menon

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

[Kevin Hilman]

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()

>> +			} 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.

Thanks,

Kevin

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

[Nishanth Menon]

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

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

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

[...]

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

doh, looks like I should've RTFM.

Thanks for the pointer.

Kevin

Re: [PATCH 3/3] OMAP: hwmod: allow idle after HWMOD_INIT_NO_IDLE

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> If an omap_hwmod is setup using HWMOD_INIT_NO_IDLE flag, there is
> currently way to idle it since omap_hwmod_idle() requires the hwmod to
> be in the enabled state.
>
> This patch adds a check to omap_hwmod_idle() so if the hwmod was has
> the INIT_NO_IDLE flag, calling omap_hwmod_idle() will still succeed.
>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Retracting this patch as it is no longer necessary after recent
hwmod updates from Paul.

Kevin

> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 307deea..2de4cc3 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -869,7 +869,13 @@ static int _enable(struct omap_hwmod *oh)
>   */
>  static int _idle(struct omap_hwmod *oh)
>  {
> -	if (oh->_state != _HWMOD_STATE_ENABLED) {
> +	/*
> +	 * To idle, hwmod must be enabled, EXCEPT if hwmod was
> +	 * initialized using the INIT_NO_IDLE flag.  In this case it
> +	 * will not yet be enabled so we have to allow it to be idled.
> +	 */
> +	if ((oh->_state != _HWMOD_STATE_ENABLED) &&
> +	    !(oh->flags & HWMOD_INIT_NO_IDLE)) {
>  		WARN(1, "omap_hwmod: %s: idle state can only be entered from "
>  		     "enabled state\n", oh->name);
>  		return -EINVAL;
> @@ -884,6 +890,9 @@ static int _idle(struct omap_hwmod *oh)
>  
>  	oh->_state = _HWMOD_STATE_IDLE;
>  
> +	/* Clear init flag which should only affect first call to idle */
> +	oh->flags &= ~HWMOD_INIT_NO_IDLE;
> +
>  	return 0;
>  }
>  
> -- 
> 1.6.6.rc2.1.g42108

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

[Paul Walmsley]

Hi Kevin,

On Fri, 8 Jan 2010, Kevin Hilman wrote:

> 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);
> +			} 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);
> +		}
>  
>  		od->dev_wakeup_lat -= odpl->activate_lat;
>  	}
> @@ -204,10 +216,23 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)
>  			 "%llu nsec\n", od->pdev.name, od->pm_lat_level,
>  			 deact_lat);
>  
> -		WARN(deact_lat > odpl->deactivate_lat, "omap_device: %s.%d: "
> -		     "deactivate step %d took longer than expected "
> -		     "(%llu > %d)\n", od->pdev.name, od->pdev.id,
> -		     od->pm_lat_level, deact_lat, odpl->deactivate_lat);
> +		if (deact_lat > odpl->deactivate_lat) {
> +			odpl->deactivate_lat_worst = deact_lat;
> +			if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
> +				odpl->deactivate_lat = deact_lat;
> +				pr_warning("omap_device: %s.%d: new worst case "
> +					   "deactivate latency %d: %llu\n",
> +					   od->pdev.name, od->pdev.id,
> +					   od->pm_lat_level, deact_lat);
> +			} else
> +				pr_warning("omap_device: %s.%d: deactivate "
> +					   "latency %d higher than exptected. "
> +					   "(%llu > %d)\n",
> +					   od->pdev.name, od->pdev.id,
> +					   od->pm_lat_level, deact_lat,
> +					   odpl->deactivate_lat);
> +		}
> +
>  
>  		od->dev_wakeup_lat += odpl->activate_lat;
>  

Thanks, looks very useful.  The no-idle thing is extremely strange and 
useful to know.

Queued for 2.6.34. 


- Paul

Re: [PATCH 2/3] OMAP: hwmod: add read/write API for SYSCONFIG

[Paul Walmsley]

Hi Kevin, 

On Fri, 8 Jan 2010, Kevin Hilman wrote:

> Some HW blocks have errata which requires selective enabling/disabling
> of SYSCONFIG bits.  In particular, some blocks have known issues with
> smart-idle so smart-idle has to be disabled under certain conditions.

...

> RFC: would an API to only touch smart-idle be more appropriate? Maybe 
> omap_hwmod_smart_idle_enable(oh, bool enable)?

This idea sounds good.  Since the SYSCONFIG bit fields can change and move 
around depending on the chip and IP, some type of higher-level API seems 
necessary to preserve sanity.

Maybe omap_hwmod_smart_idle_enable() and omap_hwmod_smart_idle_disable(), 
mimicking API styles like clk_enable()/clk_disable(), etc.?  Care to spin 
something like that?


- Paul

Re: [PATCH 2/3] OMAP: hwmod: add read/write API for SYSCONFIG

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> Hi Kevin, 
>
> On Fri, 8 Jan 2010, Kevin Hilman wrote:
>
>> Some HW blocks have errata which requires selective enabling/disabling
>> of SYSCONFIG bits.  In particular, some blocks have known issues with
>> smart-idle so smart-idle has to be disabled under certain conditions.
>
> ...
>
>> RFC: would an API to only touch smart-idle be more appropriate? Maybe 
>> omap_hwmod_smart_idle_enable(oh, bool enable)?
>
> This idea sounds good.  Since the SYSCONFIG bit fields can change and move 
> around depending on the chip and IP, some type of higher-level API seems 
> necessary to preserve sanity.
>
> Maybe omap_hwmod_smart_idle_enable() and omap_hwmod_smart_idle_disable(), 
> mimicking API styles like clk_enable()/clk_disable(), etc.?  Care to spin 
> something like that?

Sounds good, Coming right up....

Kevin

Re: [PATCH 2/3] OMAP: hwmod: add read/write API for SYSCONFIG

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> Hi Kevin, 
>
> On Fri, 8 Jan 2010, Kevin Hilman wrote:
>
>> Some HW blocks have errata which requires selective enabling/disabling
>> of SYSCONFIG bits.  In particular, some blocks have known issues with
>> smart-idle so smart-idle has to be disabled under certain conditions.
>
> ...
>
>> RFC: would an API to only touch smart-idle be more appropriate? Maybe 
>> omap_hwmod_smart_idle_enable(oh, bool enable)?
>
> This idea sounds good.  Since the SYSCONFIG bit fields can change and move 
> around depending on the chip and IP, some type of higher-level API seems 
> necessary to preserve sanity.
>
> Maybe omap_hwmod_smart_idle_enable() and omap_hwmod_smart_idle_disable(), 
> mimicking API styles like clk_enable()/clk_disable(), etc.?  Care to spin 
> something like that?

Turns out I needed more than just smart-idle enable/disable.  There
are UART errata that also require force-idle (when DMA is in use.)

So, instead I added an API for simply setting slave idle mode (patch
below.)

Here's the UART code that is using this function:
{
	u8 idlemode;

	if (enable) {
		/**
		 * Errata 2.15: [UART]:Cannot Acknowledge Idle Requests
		 * in Smartidle Mode When Configured for DMA Operations.
		 */
		if (uart->dma_enabled)
			idlemode = HWMOD_IDLEMODE_FORCE;
		else
			idlemode = HWMOD_IDLEMODE_SMART;
	} else {
		idlemode = HWMOD_IDLEMODE_NO;
	}

	omap_hwmod_set_slave_idlemode(uart->oh, idlemode);
}


>From fb0387e2747e2c0b1eff99bc842cad5026e96283 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@deeprootsystems.com>
Date: Mon, 23 Nov 2009 09:41:54 -0800
Subject: [PATCH 2/2] OMAP: hwmod: add API for slave idlemode setting

Some HW blocks have errata which requires specific slave idle mode
under certain conditions.

This patch adds an hwmod API to allow setting slave idlemode
ensuring that any SYSCONFIG register updates go through hwmod.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   17 +++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    2 ++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index d8c8545..3bc47dc 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -993,6 +993,23 @@ void omap_hwmod_writel(u32 v, struct omap_hwmod *oh, u16 reg_offs)
 	__raw_writel(v, oh->_rt_va + reg_offs);
 }
 
+int omap_hwmod_set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode)
+{
+	u32 v;
+	int retval = 0;
+
+	if (!oh)
+		return -EINVAL;
+
+	v = oh->_sysc_cache;
+
+	retval = _set_slave_idlemode(oh, idlemode, &v);
+	if (!retval)
+		_write_sysconfig(v, oh);
+
+	return retval;
+}
+
 /**
  * omap_hwmod_register - register a struct omap_hwmod
  * @oh: struct omap_hwmod *
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 007935a..1f57330 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -440,6 +440,8 @@ int omap_hwmod_shutdown(struct omap_hwmod *oh);
 int omap_hwmod_enable_clocks(struct omap_hwmod *oh);
 int omap_hwmod_disable_clocks(struct omap_hwmod *oh);
 
+int omap_hwmod_set_slave_idlemode(struct omap_hwmod *oh, u8 idlemode);
+
 int omap_hwmod_reset(struct omap_hwmod *oh);
 void omap_hwmod_ocp_barrier(struct omap_hwmod *oh);
 
-- 
1.6.6

Re: [PATCH 2/3] OMAP: hwmod: add read/write API for SYSCONFIG

[Paul Walmsley]

On Thu, 14 Jan 2010, Kevin Hilman wrote:

> Turns out I needed more than just smart-idle enable/disable.  There
> are UART errata that also require force-idle (when DMA is in use.)
> 
> So, instead I added an API for simply setting slave idle mode (patch
> below.)

Thanks Kevin, queued for 2.6.34.


- Paul