* RE: [RFC][Patch] OMAP3 PM: sysfs vdd_opp change to use Constraint Framework
2009-10-16 19:26 [RFC][Patch] OMAP3 PM: sysfs vdd_opp change to use Constraint Framework Sripathy, Vishwanath
@ 2009-10-16 19:27 ` Sripathy, Vishwanath
2009-10-20 15:51 ` Kevin Hilman
1 sibling, 0 replies; 5+ messages in thread
From: Sripathy, Vishwanath @ 2009-10-16 19:27 UTC (permalink / raw)
To: Sripathy, Vishwanath, linux-omap@vger.kernel.org, Hilman, Kevin
[-- Attachment #1: Type: text/plain, Size: 4085 bytes --]
Note that this patch has dependency on Patch sent by Romit (in attached email).
Vishwanath
-----Original Message-----
From: Sripathy, Vishwanath
Sent: Friday, October 16, 2009 2:26 PM
To: Sripathy, Vishwanath; linux-omap@vger.kernel.org; Hilman, Kevin
Subject: [RFC][Patch] OMAP3 PM: sysfs vdd_opp change to use Constraint Framework
Sysfs entry vdd_opp currently does not use constraint framework.
Because of this, even if you set the opp via this entry, it has no effect
since it can be overridden by on demand governor any time.
This patch has changes to make vdd_opp to use constraint framework.
vdd_lock variable has to be used when application wants to disable DVFS
altogether. This will override any other constraints and will lock the
OPP to given value.
Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
---
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 13783f3..bcdc00d 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -47,6 +47,8 @@ static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
static ssize_t vdd_opp_show(struct kobject *, struct kobj_attribute *, char *);
static ssize_t vdd_opp_store(struct kobject *k, struct kobj_attribute *,
const char *buf, size_t n);
+static ssize_t vdd_opp_lock_store(struct kobject *k, struct kobj_attribute *,
+ const char *buf, size_t n);
static struct kobj_attribute vdd1_opp_attr =
__ATTR(vdd1_opp, 0644, vdd_opp_show, vdd_opp_store);
@@ -103,6 +105,41 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
const char *buf, size_t n)
{
unsigned short value;
+ struct omap_opp *opp_table;
+
+ if (sscanf(buf, "%hu", &value) != 1)
+ return -EINVAL;
+
+ if (attr == &vdd1_opp_attr) {
+ if (value < MIN_VDD1_OPP || value > MAX_VDD1_OPP) {
+ printk(KERN_ERR "vdd_opp_store: Invalid value\n");
+ return -EINVAL;
+ }
+ opp_table = omap_get_mpu_rate_table();
+ omap_pm_cpu_set_freq(&sysfs_cpufreq_dev,
+ opp_table[value].rate);
+ } else if (attr == &vdd2_opp_attr) {
+ if (value < MIN_VDD2_OPP || (value > MAX_VDD2_OPP)) {
+ printk(KERN_ERR "vdd_opp_store: Invalid value\n");
+ return -EINVAL;
+ }
+ if (value == VDD2_OPP2)
+ omap_pm_set_min_bus_tput(&sysfs_cpufreq_dev,
+ OCP_INITIATOR_AGENT, 83*1000*4);
+ else if (value == VDD2_OPP3)
+ omap_pm_set_min_bus_tput(&sysfs_cpufreq_dev,
+ OCP_INITIATOR_AGENT, 166*1000*4);
+
+ } else {
+ return -EINVAL;
+ }
+ return n;
+}
+
+static ssize_t vdd_opp_lock_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t n)
+{
+ unsigned short value;
int flags = 0;
if (sscanf(buf, "%hu", &value) != 1)
@@ -121,6 +158,11 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
if (vdd1_locked == 0 && value != 0) {
resource_lock_opp(VDD1_OPP);
vdd1_locked = 1;
+ if (value < MIN_VDD1_OPP || value > MAX_VDD1_OPP) {
+ printk(KERN_ERR "vdd_opp_store: Invalid value\n");
+ return -EINVAL;
+ }
+ resource_set_opp_level(VDD1_OPP, value, flags);
}
} else if (attr == &vdd2_lock_attr) {
flags = OPP_IGNORE_LOCK;
@@ -134,21 +176,12 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
if (vdd2_locked == 0 && value != 0) {
resource_lock_opp(VDD2_OPP);
vdd2_locked = 1;
+ if (value < MIN_VDD2_OPP || value > MAX_VDD2_OPP) {
+ printk(KERN_ERR "vdd_opp_store: Invalid value\n");
+ return -EINVAL;
+ }
+ resource_set_opp_level(VDD2_OPP, value, flags);
}
- }
-
- if (attr == &vdd1_opp_attr) {
- if (value < 1 || value > 5) {
- printk(KERN_ERR "vdd_opp_store: Invalid value\n");
- return -EINVAL;
- }
- resource_set_opp_level(VDD1_OPP, value, flags);
- } else if (attr == &vdd2_opp_attr) {
- if (value < 2 || value > 3) {
- printk(KERN_ERR "vdd_opp_store: Invalid value\n");
- return -EINVAL;
- }
- resource_set_opp_level(VDD2_OPP, value, flags);
} else {
return -EINVAL;
}
[-- Attachment #2: Type: message/rfc822, Size: 9284 bytes --]
From: "Dasgupta, Romit" <romit@ti.com>
To: Kevin H <khilman@deeprootsystems.com>
Cc: "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>
Subject: [PATCH] omap-pm: Fixes behaviour of some shared resource framework functions
Date: Wed, 7 Oct 2009 18:11:50 +0530
Message-ID: <B85A65D85D7EB246BE421B3FB0FBB59301DDCE4650@dbde02.ent.ti.com>
(Tested on Zoom2).
'omap_pm_dsp_set_min_opp' & 'omap_pm_cpu_set_freq' were using their own
struct device *. This is a problem because invoking these functions from
different clients would result in setting of the resource level as requested by
the last caller. Fixes this by introducing a struct device * to the parameter
list for these functions.
Signed-off-by: Romit Dasgupta <romit@ti.com>
---
diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c
index 8e67861..e9adc67 100644
--- a/arch/arm/plat-omap/cpu-omap.c
+++ b/arch/arm/plat-omap/cpu-omap.c
@@ -47,6 +47,7 @@ static struct cpufreq_frequency_table *freq_table;
#define MPU_CLK "virt_prcm_set"
#endif
+DEFINE_PER_CPU(struct device , cpu_freq_dev);
static struct clk *mpu_clk;
/* TODO: Add support for SDRAM timing changes */
@@ -115,8 +116,9 @@ static int omap_target(struct cpufreq_policy *policy,
int ind;
for (ind = 1; ind <= MAX_VDD1_OPP; ind++) {
if (mpu_opps[ind].rate/1000 >= target_freq) {
- omap_pm_cpu_set_freq
- (mpu_opps[ind].rate);
+ omap_pm_cpu_set_freq(
+ &__get_cpu_var(cpu_freq_dev),
+ mpu_opps[ind].rate);
break;
}
}
diff --git a/arch/arm/plat-omap/include/mach/omap-pm.h b/arch/arm/plat-omap/include/mach/omap-pm.h
index 583e540..5b26ba1 100644
--- a/arch/arm/plat-omap/include/mach/omap-pm.h
+++ b/arch/arm/plat-omap/include/mach/omap-pm.h
@@ -226,6 +226,7 @@ const struct omap_opp *omap_pm_dsp_get_opp_table(void);
/**
* omap_pm_dsp_set_min_opp - receive desired OPP target ID from DSP Bridge
+ * @dev: Identifies the client that wants to set the VDD1 OPP.
* @opp_id: target DSP OPP ID
*
* Set a minimum OPP ID for the DSP. This is intended to be called
@@ -233,7 +234,7 @@ const struct omap_opp *omap_pm_dsp_get_opp_table(void);
* information that code receives from the DSP/BIOS load estimator is the
* target OPP ID; hence, this interface. No return value.
*/
-void omap_pm_dsp_set_min_opp(u8 opp_id);
+void omap_pm_dsp_set_min_opp(struct device *dev, u8 opp_id);
/**
* omap_pm_dsp_get_opp - report the current DSP OPP ID
@@ -283,6 +284,7 @@ struct cpufreq_frequency_table **omap_pm_cpu_get_freq_table(void);
/**
* omap_pm_cpu_set_freq - set the current minimum MPU frequency
+ * @dev: Identifies the client that wants to set the frequency.
* @f: MPU frequency in Hz
*
* Set the current minimum CPU frequency. The actual CPU frequency
@@ -290,7 +292,7 @@ struct cpufreq_frequency_table **omap_pm_cpu_get_freq_table(void);
* Intended to be called by plat-omap/cpu_omap.c:omap_target(). No
* return value.
*/
-void omap_pm_cpu_set_freq(unsigned long f);
+void omap_pm_cpu_set_freq(struct device *dev, unsigned long f);
/**
* omap_pm_cpu_get_freq - report the current CPU frequency
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index 10463a4..6546527 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -159,7 +159,7 @@ const struct omap_opp *omap_pm_dsp_get_opp_table(void)
}
EXPORT_SYMBOL(omap_pm_dsp_get_opp_table);
-void omap_pm_dsp_set_min_opp(u8 opp_id)
+void omap_pm_dsp_set_min_opp(struct device *dev, u8 opp_id)
{
if (opp_id == 0) {
WARN_ON(1);
@@ -244,7 +244,7 @@ struct cpufreq_frequency_table **omap_pm_cpu_get_freq_table(void)
return NULL;
}
-void omap_pm_cpu_set_freq(unsigned long f)
+void omap_pm_cpu_set_freq(struct device *dev, unsigned long f)
{
if (f == 0) {
WARN_ON(1);
diff --git a/arch/arm/plat-omap/omap-pm-srf.c b/arch/arm/plat-omap/omap-pm-srf.c
index 4350650..aece740 100644
--- a/arch/arm/plat-omap/omap-pm-srf.c
+++ b/arch/arm/plat-omap/omap-pm-srf.c
@@ -169,8 +169,6 @@ void omap_pm_set_max_sdma_lat(struct device *dev, long t)
}
}
-static struct device dummy_dsp_dev;
-
/*
* DSP Bridge-specific constraints
*/
@@ -187,20 +185,15 @@ const struct omap_opp *omap_pm_dsp_get_opp_table(void)
}
EXPORT_SYMBOL(omap_pm_dsp_get_opp_table);
-void omap_pm_dsp_set_min_opp(u8 opp_id)
+void omap_pm_dsp_set_min_opp(struct device *dev, u8 opp_id)
{
- if (opp_id == 0) {
+ if (dev == NULL || opp_id == 0) {
WARN_ON(1);
return;
}
pr_debug("OMAP PM: DSP requests minimum VDD1 OPP to be %d\n", opp_id);
-
- /*
- * For now pass a dummy_dev struct for SRF to identify the caller.
- * Maybe its good to have DSP pass this as an argument
- */
- resource_request("vdd1_opp", &dummy_dsp_dev, opp_id);
+ resource_request("vdd1_opp", dev, opp_id);
return;
}
EXPORT_SYMBOL(omap_pm_dsp_set_min_opp);
@@ -246,19 +239,16 @@ struct cpufreq_frequency_table **omap_pm_cpu_get_freq_table(void)
return NULL;
}
-static struct device dummy_cpufreq_dev;
-
-void omap_pm_cpu_set_freq(unsigned long f)
+void omap_pm_cpu_set_freq(struct device *dev, unsigned long f)
{
- if (f == 0) {
+ if (dev == NULL || f == 0) {
WARN_ON(1);
return;
}
pr_debug("OMAP PM: CPUFreq requests CPU frequency to be set to %lu\n",
f);
-
- resource_request("mpu_freq", &dummy_cpufreq_dev, f);
+ resource_request("mpu_freq", dev, f);
return;
}
EXPORT_SYMBOL(omap_pm_cpu_set_freq);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC][Patch] OMAP3 PM: sysfs vdd_opp change to use Constraint Framework
2009-10-16 19:26 [RFC][Patch] OMAP3 PM: sysfs vdd_opp change to use Constraint Framework Sripathy, Vishwanath
2009-10-16 19:27 ` Sripathy, Vishwanath
@ 2009-10-20 15:51 ` Kevin Hilman
2009-10-20 17:02 ` Tony Lindgren
1 sibling, 1 reply; 5+ messages in thread
From: Kevin Hilman @ 2009-10-20 15:51 UTC (permalink / raw)
To: Sripathy, Vishwanath; +Cc: linux-omap@vger.kernel.org
"Sripathy, Vishwanath" <vishwanath.bs@ti.com> writes:
> Sysfs entry vdd_opp currently does not use constraint framework.
> Because of this, even if you set the opp via this entry, it has no effect
> since it can be overridden by on demand governor any time.
> This patch has changes to make vdd_opp to use constraint framework.
> vdd_lock variable has to be used when application wants to disable DVFS
> altogether. This will override any other constraints and will lock the
> OPP to given value.
>
> Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
This raises an issue that I've been thinking about for awhile.
Basically, I'd like to drop all the /sys/power/vdd* options.
Since we can do the equivalent with CPUFreq using the userspace
governor, I don't see the point of having an additional interface.
Comments?
Kevin
> ---
>
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 13783f3..bcdc00d 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -47,6 +47,8 @@ static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
> static ssize_t vdd_opp_show(struct kobject *, struct kobj_attribute *, char *);
> static ssize_t vdd_opp_store(struct kobject *k, struct kobj_attribute *,
> const char *buf, size_t n);
> +static ssize_t vdd_opp_lock_store(struct kobject *k, struct kobj_attribute *,
> + const char *buf, size_t n);
> static struct kobj_attribute vdd1_opp_attr =
> __ATTR(vdd1_opp, 0644, vdd_opp_show, vdd_opp_store);
>
> @@ -103,6 +105,41 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
> const char *buf, size_t n)
> {
> unsigned short value;
> + struct omap_opp *opp_table;
> +
> + if (sscanf(buf, "%hu", &value) != 1)
> + return -EINVAL;
> +
> + if (attr == &vdd1_opp_attr) {
> + if (value < MIN_VDD1_OPP || value > MAX_VDD1_OPP) {
> + printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> + return -EINVAL;
> + }
> + opp_table = omap_get_mpu_rate_table();
> + omap_pm_cpu_set_freq(&sysfs_cpufreq_dev,
> + opp_table[value].rate);
> + } else if (attr == &vdd2_opp_attr) {
> + if (value < MIN_VDD2_OPP || (value > MAX_VDD2_OPP)) {
> + printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> + return -EINVAL;
> + }
> + if (value == VDD2_OPP2)
> + omap_pm_set_min_bus_tput(&sysfs_cpufreq_dev,
> + OCP_INITIATOR_AGENT, 83*1000*4);
> + else if (value == VDD2_OPP3)
> + omap_pm_set_min_bus_tput(&sysfs_cpufreq_dev,
> + OCP_INITIATOR_AGENT, 166*1000*4);
> +
> + } else {
> + return -EINVAL;
> + }
> + return n;
> +}
> +
> +static ssize_t vdd_opp_lock_store(struct kobject *kobj,
> + struct kobj_attribute *attr, const char *buf, size_t n)
> +{
> + unsigned short value;
> int flags = 0;
>
> if (sscanf(buf, "%hu", &value) != 1)
> @@ -121,6 +158,11 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (vdd1_locked == 0 && value != 0) {
> resource_lock_opp(VDD1_OPP);
> vdd1_locked = 1;
> + if (value < MIN_VDD1_OPP || value > MAX_VDD1_OPP) {
> + printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> + return -EINVAL;
> + }
> + resource_set_opp_level(VDD1_OPP, value, flags);
> }
> } else if (attr == &vdd2_lock_attr) {
> flags = OPP_IGNORE_LOCK;
> @@ -134,21 +176,12 @@ static ssize_t vdd_opp_store(struct kobject *kobj, struct kobj_attribute *attr,
> if (vdd2_locked == 0 && value != 0) {
> resource_lock_opp(VDD2_OPP);
> vdd2_locked = 1;
> + if (value < MIN_VDD2_OPP || value > MAX_VDD2_OPP) {
> + printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> + return -EINVAL;
> + }
> + resource_set_opp_level(VDD2_OPP, value, flags);
> }
> - }
> -
> - if (attr == &vdd1_opp_attr) {
> - if (value < 1 || value > 5) {
> - printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> - return -EINVAL;
> - }
> - resource_set_opp_level(VDD1_OPP, value, flags);
> - } else if (attr == &vdd2_opp_attr) {
> - if (value < 2 || value > 3) {
> - printk(KERN_ERR "vdd_opp_store: Invalid value\n");
> - return -EINVAL;
> - }
> - resource_set_opp_level(VDD2_OPP, value, flags);
> } else {
> return -EINVAL;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread