[PATCH 0/5] Thermal: cpu cooling fix

[PATCH 0/5] Thermal: cpu cooling fix

[Zhang Rui]

the per_cpu cpufreq_frequency_table contains all the frequency entries
which are reported by platform code.
But this table is not clean enough because it may contain invalid,
duplicate entries, and it may be in either descending or asceding order.

converting between cpu frequency and cooling state using this table
in cpufreq cooling code is painful as each cooling state of
cpu cooling device must have a valid and unique frequency.

thus cpu cooling table is introduced in this patch set.
CPU cooling table is a table that
1. each entry represents a cpu cooling state,
   aka, a different cpu frequency.
2. no invalid entry in this table
3. no duplicate entry in this table
4. the frequency of all the entries are in descending order, aka,
   table[0] equals maximum frequency and cooling state 0.

And this is the table we are using inside cpufreq cooling generic code.

Besides, all the cpufreq cooling users, e.g. exynos, should touch
neither per_cpu cpufreq_frequency_table nor the cpu cooling table.
it should invoke cpufreq cooling API to do cooling_state/frequency
transition.

I just run build test here. It would be great that some of you
can test it on exynos.
any comments?

thanks,
rui

[PATCH 1/5] Thermal: fix a wrong comment

[Zhang Rui]

"level" parameter of get_cpu_frequency equals cooling state
of cpu cooling device, and it starts from 0.

Fix the misleading comment.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/cpu_cooling.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 836828e..455c77a 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -118,8 +118,8 @@ static int is_cpufreq_valid(int cpu)
 /**
  * get_cpu_frequency - get the absolute value of frequency from level.
  * @cpu: cpu for which frequency is fetched.
- * @level: level of frequency of the CPU
- *	e.g level=1 --> 1st MAX FREQ, LEVEL=2 ---> 2nd MAX FREQ, .... etc
+ * @level: level of frequency, equals cooling state of cpu cooling device
+ *	e.g level=0 --> 1st MAX FREQ, level=1 ---> 2nd MAX FREQ, .... etc
  */
 static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 {
-- 
1.7.9.5

[PATCH 2/5] Thermal: Introduce cpu cooling table

[Zhang Rui]

CPU cooling table is a cleaned cpufreq_frequency_table that
1. each entry represents a cooling state,
   aka, each entry has a different frequency.
2. does not have invalid entry.
3. does not have duplicate entry.
4. the frequencies of all the entries are in descending order.

we should use this table inside thermal layer and thermal drivers.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 455c77a..08f12c7 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
 	return !cpufreq_get_policy(&policy, cpu);
 }
 
+/*
+ * get the cleaned cpufreq_frequency_table that
+ * 1) does not have invalid entries
+ * 2) does not have duplicate entries
+ * 3) the frequency of the entries are in descending order
+ */
+static struct cpufreq_frequency_table *
+get_cpu_cooling_table(unsigned int cpu)
+{
+	int i = 0;
+	int level;
+	struct cpufreq_frequency_table *old, *new;
+	unsigned int freq = CPUFREQ_ENTRY_INVALID;
+	int descend = -1;
+
+	old = cpufreq_frequency_get_table(cpu);
+	if (!old)
+		return ERR_PTR(-EINVAL);
+
+	while (old[i].frequency != CPUFREQ_TABLE_END)
+		i++;
+
+	i++; /* one more entry for CPUFREQ_TABLE_END */
+
+	new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
+		/* ignore invalid entry */
+		if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
+			continue;
+
+		/* ignore duplicate entry */
+		if (freq == old[i].frequency)
+			continue;
+
+		/* found an valid entry */
+		new[level].frequency = old[i].frequency;
+
+		if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
+			descend = !!(freq > old[i].frequency);
+
+		/* freq always equals the last valid frequency */
+		freq = new[level].frequency;
+
+		level++;
+	}
+	new[level].frequency = CPUFREQ_ENTRY_INVALID;
+
+	/* convert to decending if in ascending order */
+	if (!descend) {
+		for(i = 0; i < (level - i - 1); i++) {
+			int j = level - i - 1;
+			freq = new[i].frequency;
+			new[i].frequency = new[j].frequency;
+			new[j].frequency = freq;
+		}
+	}
+
+	return new;
+}
+
 /**
  * get_cpu_frequency - get the absolute value of frequency from level.
  * @cpu: cpu for which frequency is fetched.
-- 
1.7.9.5

[PATCH 3/5] Thermal: Get cpu frequency via cpu cooling table

[Zhang Rui]

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/cpu_cooling.c |   45 ++++++++---------------------------------
 1 file changed, 8 insertions(+), 37 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 08f12c7..57bb669 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -186,46 +186,17 @@ get_cpu_cooling_table(unsigned int cpu)
  */
 static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
 {
-	int ret = 0, i = 0;
-	unsigned long level_index;
-	bool descend = false;
-	struct cpufreq_frequency_table *table =
-					cpufreq_frequency_get_table(cpu);
-	if (!table)
-		return ret;
-
-	while (table[i].frequency != CPUFREQ_TABLE_END) {
-		if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
-			continue;
-
-		/*check if table in ascending or descending order*/
-		if ((table[i + 1].frequency != CPUFREQ_TABLE_END) &&
-			(table[i + 1].frequency < table[i].frequency)
-			&& !descend) {
-			descend = true;
-		}
+	struct cpufreq_frequency_table *table;
+	unsigned int freq;
 
-		/*return if level matched and table in descending order*/
-		if (descend && i == level)
-			return table[i].frequency;
-		i++;
-	}
-	i--;
+	table = get_cpu_cooling_table(cpu);
+	if (IS_ERR(table))
+		return -EINVAL;
 
-	if (level > i || descend)
-		return ret;
-	level_index = i - level;
+	freq = table[level].frequency;
 
-	/*Scan the table in reverse order and match the level*/
-	while (i >= 0) {
-		if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
-			continue;
-		/*return if level matched*/
-		if (i == level_index)
-			return table[i].frequency;
-		i--;
-	}
-	return ret;
+	kfree(table);
+	return 0;
 }
 
 /**
-- 
1.7.9.5

[PATCH 4/5] Thermal: Get max cpu cooling states via cpu cooling table

[Zhang Rui]

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/cpu_cooling.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 57bb669..784be30 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -277,26 +277,22 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev,
 {
 	struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
 	struct cpumask *maskPtr = &cpufreq_device->allowed_cpus;
-	unsigned int cpu;
 	struct cpufreq_frequency_table *table;
-	unsigned long count = 0;
+	unsigned int cpu;
 	int i = 0;
 
 	cpu = cpumask_any(maskPtr);
-	table = cpufreq_frequency_get_table(cpu);
-	if (!table) {
-		*state = 0;
-		return 0;
-	}
+	table = get_cpu_cooling_table(cpu);
+	if (IS_ERR(table))
+		return -EINVAL;
 
-	for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
-		if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
-			continue;
-		count++;
-	}
+	while (table[i].frequency != CPUFREQ_TABLE_END)
+		i++;
+
+	kfree(table);
 
-	if (count > 0) {
-		*state = --count;
+	if (i > 0) {
+		*state = --i;
 		return 0;
 	}
 
-- 
1.7.9.5

[PATCH 5/5] Thermal: Get cooling state via cpu cooling table

[Zhang Rui]

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/cpu_cooling.c    |   22 ++++++++++++++++++++++
 drivers/thermal/exynos_thermal.c |   24 ++----------------------
 include/linux/cpu_cooling.h      |    6 ++++++
 include/linux/thermal.h          |    4 +++-
 4 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 784be30..9dbcbc1 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -178,6 +178,28 @@ get_cpu_cooling_table(unsigned int cpu)
 	return new;
 }
 
+unsigned long cpufreq_cooling_get_level(unsigned int cpu,
+			unsigned int freq)
+{
+	struct cpufreq_frequency_table *table;
+	int i;
+	unsigned long level = THERMAL_COOLING_STATE_INVALID;
+
+	table = get_cpu_cooling_table(cpu);
+	if (IS_ERR(table))
+		return -EINVAL;
+
+	for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++)
+		if (freq == table[i].frequency) {
+			level = (unsigned long)i;
+			break;
+		}
+
+	kfree(table);
+	return level;
+}
+EXPORT_SYMBOL(cpufreq_cooling_get_level);
+
 /**
  * get_cpu_frequency - get the absolute value of frequency from level.
  * @cpu: cpu for which frequency is fetched.
diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
index cd71e24..2fea7a9 100644
--- a/drivers/thermal/exynos_thermal.c
+++ b/drivers/thermal/exynos_thermal.c
@@ -239,26 +239,6 @@ static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
 	return ret;
 }
 
-static int exynos_get_frequency_level(unsigned int cpu, unsigned int freq)
-{
-	int i = 0, ret = -EINVAL;
-	struct cpufreq_frequency_table *table = NULL;
-#ifdef CONFIG_CPU_FREQ
-	table = cpufreq_frequency_get_table(cpu);
-#endif
-	if (!table)
-		return ret;
-
-	while (table[i].frequency != CPUFREQ_TABLE_END) {
-		if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
-			continue;
-		if (table[i].frequency == freq)
-			return i;
-		i++;
-	}
-	return ret;
-}
-
 /* Bind callback functions for thermal zone */
 static int exynos_bind(struct thermal_zone_device *thermal,
 			struct thermal_cooling_device *cdev)
@@ -285,8 +265,8 @@ static int exynos_bind(struct thermal_zone_device *thermal,
 	/* Bind the thermal zone to the cpufreq cooling device */
 	for (i = 0; i < tab_size; i++) {
 		clip_data = (struct freq_clip_table *)&(tab_ptr[i]);
-		level = exynos_get_frequency_level(0, clip_data->freq_clip_max);
-		if (level < 0)
+		level = cpufreq_cooling_get_level(0, clip_data->freq_clip_max);
+		if (level == THERMAL_COOLING_STATE_INVALID)
 			return 0;
 		switch (GET_ZONE(i)) {
 		case MONITOR_ZONE:
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
index 40b4ef5..0a4a05d 100644
--- a/include/linux/cpu_cooling.h
+++ b/include/linux/cpu_cooling.h
@@ -42,6 +42,7 @@ struct thermal_cooling_device *cpufreq_cooling_register(
  * @cdev: thermal cooling device pointer.
  */
 void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
+unsigned long cpufreq_cooling_get_level(unsigned int, unsigned int);
 #else /* !CONFIG_CPU_THERMAL */
 static inline struct thermal_cooling_device *cpufreq_cooling_register(
 	const struct cpumask *clip_cpus)
@@ -53,6 +54,11 @@ static inline void cpufreq_cooling_unregister(
 {
 	return;
 }
+static inline unsigned long cpufreq_cooling_get_level(
+		unsigned int cpu, unsigned int freq)
+{
+	return THERMAL_COOLING_STATE_INVALID;
+}
 #endif	/* CONFIG_CPU_THERMAL */
 
 #endif /* __CPU_COOLING_H__ */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index f0bd7f9..03bdaa4 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -33,8 +33,10 @@
 #define THERMAL_MAX_TRIPS	12
 #define THERMAL_NAME_LENGTH	20
 
+#define THERMAL_COOLING_STATE_INVALID -1UL
+
 /* No upper/lower limit requirement */
-#define THERMAL_NO_LIMIT	-1UL
+#define THERMAL_NO_LIMIT THERMAL_COOLING_STATE_INVALID
 
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)	(long)(((long)t-2732 >= 0) ?	\
-- 
1.7.9.5

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[Hongbo Zhang]

On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote:
> CPU cooling table is a cleaned cpufreq_frequency_table that
> 1. each entry represents a cooling state,
>    aka, each entry has a different frequency.
> 2. does not have invalid entry.
> 3. does not have duplicate entry.
> 4. the frequencies of all the entries are in descending order.
>
This is a good idea, but we should consider more details, one tough
issue is invalid frequencies:
1. some freq is invalid while freq table is creating, but after that
the invalid freq becomes valid, we will miss cooling states.
2. vice versa, all the freq is valid while freq table is creating, but
become invalid later, thus some cooling state isn't valid either,
should we consider such case?

In fact, these should be considered even if this patch isn't sent out.

(We are assuming that all the CPU cores run at same freq and should be
adjust freq simultaneously, if no affinity between CPU cores, it will
be more complicated -- this seems not proper to be discussed in this
patch)

> we should use this table inside thermal layer and thermal drivers.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 455c77a..08f12c7 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
>         return !cpufreq_get_policy(&policy, cpu);
>  }
>
> +/*
> + * get the cleaned cpufreq_frequency_table that
> + * 1) does not have invalid entries
> + * 2) does not have duplicate entries
> + * 3) the frequency of the entries are in descending order
> + */
> +static struct cpufreq_frequency_table *
> +get_cpu_cooling_table(unsigned int cpu)
> +{
> +       int i = 0;
> +       int level;
> +       struct cpufreq_frequency_table *old, *new;
> +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
> +       int descend = -1;
> +
> +       old = cpufreq_frequency_get_table(cpu);
> +       if (!old)
> +               return ERR_PTR(-EINVAL);
> +
> +       while (old[i].frequency != CPUFREQ_TABLE_END)
> +               i++;
> +
> +       i++; /* one more entry for CPUFREQ_TABLE_END */
> +
> +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
> +       if (!new)
> +               return ERR_PTR(-ENOMEM);
> +
> +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
> +               /* ignore invalid entry */
> +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
> +                       continue;
> +
> +               /* ignore duplicate entry */
> +               if (freq == old[i].frequency)
> +                       continue;
> +
> +               /* found an valid entry */
> +               new[level].frequency = old[i].frequency;
> +
> +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
> +                       descend = !!(freq > old[i].frequency);
> +
> +               /* freq always equals the last valid frequency */
> +               freq = new[level].frequency;
> +
> +               level++;
> +       }
> +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
> +
> +       /* convert to decending if in ascending order */
> +       if (!descend) {
> +               for(i = 0; i < (level - i - 1); i++) {
> +                       int j = level - i - 1;
> +                       freq = new[i].frequency;
> +                       new[i].frequency = new[j].frequency;
> +                       new[j].frequency = freq;
> +               }
> +       }
> +
> +       return new;
> +}
> +
>  /**
>   * get_cpu_frequency - get the absolute value of frequency from level.
>   * @cpu: cpu for which frequency is fetched.
> --
> 1.7.9.5
>

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[Zhang Rui]

On Thu, 2013-02-07 at 17:36 +0800, Hongbo Zhang wrote:
> On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote:
> > CPU cooling table is a cleaned cpufreq_frequency_table that
> > 1. each entry represents a cooling state,
> >    aka, each entry has a different frequency.
> > 2. does not have invalid entry.
> > 3. does not have duplicate entry.
> > 4. the frequencies of all the entries are in descending order.
> >
> This is a good idea, but we should consider more details, one tough
> issue is invalid frequencies:
> 1. some freq is invalid while freq table is creating, but after that
> the invalid freq becomes valid, we will miss cooling states.

when will the invalid freq becomes valid?
for ACPI platforms, the number of valid cpu frequency should not change
at runtime, but BIOS will indicate that only several of them can be used
dynamically.
I'm not sure if this may behave differently on other platforms.
if it is true, the max_state and cur_state may change from time to time.
and we should cache current frequency rather than cur_state in cpu
cooling code.

> 2. vice versa, all the freq is valid while freq table is creating, but
> become invalid later, thus some cooling state isn't valid either,
> should we consider such case?

> In fact, these should be considered even if this patch isn't sent out.
> 
> (We are assuming that all the CPU cores run at same freq and should be
> adjust freq simultaneously, if no affinity between CPU cores, it will
> be more complicated -- this seems not proper to be discussed in this
> patch)
> 
Agreed.
But I'll try to fix the duplicate/invalid entries and ascending order
issue in this patch set.

thanks,
rui

> > we should use this table inside thermal layer and thermal drivers.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index 455c77a..08f12c7 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
> >         return !cpufreq_get_policy(&policy, cpu);
> >  }
> >
> > +/*
> > + * get the cleaned cpufreq_frequency_table that
> > + * 1) does not have invalid entries
> > + * 2) does not have duplicate entries
> > + * 3) the frequency of the entries are in descending order
> > + */
> > +static struct cpufreq_frequency_table *
> > +get_cpu_cooling_table(unsigned int cpu)
> > +{
> > +       int i = 0;
> > +       int level;
> > +       struct cpufreq_frequency_table *old, *new;
> > +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
> > +       int descend = -1;
> > +
> > +       old = cpufreq_frequency_get_table(cpu);
> > +       if (!old)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       while (old[i].frequency != CPUFREQ_TABLE_END)
> > +               i++;
> > +
> > +       i++; /* one more entry for CPUFREQ_TABLE_END */
> > +
> > +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
> > +       if (!new)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
> > +               /* ignore invalid entry */
> > +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
> > +                       continue;
> > +
> > +               /* ignore duplicate entry */
> > +               if (freq == old[i].frequency)
> > +                       continue;
> > +
> > +               /* found an valid entry */
> > +               new[level].frequency = old[i].frequency;
> > +
> > +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
> > +                       descend = !!(freq > old[i].frequency);
> > +
> > +               /* freq always equals the last valid frequency */
> > +               freq = new[level].frequency;
> > +
> > +               level++;
> > +       }
> > +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
> > +
> > +       /* convert to decending if in ascending order */
> > +       if (!descend) {
> > +               for(i = 0; i < (level - i - 1); i++) {
> > +                       int j = level - i - 1;
> > +                       freq = new[i].frequency;
> > +                       new[i].frequency = new[j].frequency;
> > +                       new[j].frequency = freq;
> > +               }
> > +       }
> > +
> > +       return new;
> > +}
> > +
> >  /**
> >   * get_cpu_frequency - get the absolute value of frequency from level.
> >   * @cpu: cpu for which frequency is fetched.
> > --
> > 1.7.9.5
> >

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[Hongbo Zhang]

On 8 February 2013 11:30, Zhang Rui <rui.zhang@intel.com> wrote:
> On Thu, 2013-02-07 at 17:36 +0800, Hongbo Zhang wrote:
>> On 6 February 2013 12:30, Zhang Rui <rui.zhang@intel.com> wrote:
>> > CPU cooling table is a cleaned cpufreq_frequency_table that
>> > 1. each entry represents a cooling state,
>> >    aka, each entry has a different frequency.
>> > 2. does not have invalid entry.
>> > 3. does not have duplicate entry.
>> > 4. the frequencies of all the entries are in descending order.
>> >
>> This is a good idea, but we should consider more details, one tough
>> issue is invalid frequencies:
>> 1. some freq is invalid while freq table is creating, but after that
>> the invalid freq becomes valid, we will miss cooling states.
>
> when will the invalid freq becomes valid?
> for ACPI platforms, the number of valid cpu frequency should not change
> at runtime, but BIOS will indicate that only several of them can be used
> dynamically.
> I'm not sure if this may behave differently on other platforms.
> if it is true, the max_state and cur_state may change from time to time.
> and we should cache current frequency rather than cur_state in cpu
> cooling code.
>
Honestly, I am not expert of cpufreq, just know that we can set
scaling_max_freq and scaling_min_freq under
/sys/devices/system/cpu/cpu*/cpufreq/, so the cpufreq scaling range
can be changed.
Correct me if I am wrong.

>> 2. vice versa, all the freq is valid while freq table is creating, but
>> become invalid later, thus some cooling state isn't valid either,
>> should we consider such case?
>
>> In fact, these should be considered even if this patch isn't sent out.
>>
>> (We are assuming that all the CPU cores run at same freq and should be
>> adjust freq simultaneously, if no affinity between CPU cores, it will
>> be more complicated -- this seems not proper to be discussed in this
>> patch)
>>
> Agreed.
> But I'll try to fix the duplicate/invalid entries and ascending order
> issue in this patch set.
>
> thanks,
> rui
>
>> > we should use this table inside thermal layer and thermal drivers.
>> >
>> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> > ---
>> >  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 63 insertions(+)
>> >
>> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> > index 455c77a..08f12c7 100644
>> > --- a/drivers/thermal/cpu_cooling.c
>> > +++ b/drivers/thermal/cpu_cooling.c
>> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
>> >         return !cpufreq_get_policy(&policy, cpu);
>> >  }
>> >
>> > +/*
>> > + * get the cleaned cpufreq_frequency_table that
>> > + * 1) does not have invalid entries
>> > + * 2) does not have duplicate entries
>> > + * 3) the frequency of the entries are in descending order
>> > + */
>> > +static struct cpufreq_frequency_table *
>> > +get_cpu_cooling_table(unsigned int cpu)
>> > +{
>> > +       int i = 0;
>> > +       int level;
>> > +       struct cpufreq_frequency_table *old, *new;
>> > +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
>> > +       int descend = -1;
>> > +
>> > +       old = cpufreq_frequency_get_table(cpu);
>> > +       if (!old)
>> > +               return ERR_PTR(-EINVAL);
>> > +
>> > +       while (old[i].frequency != CPUFREQ_TABLE_END)
>> > +               i++;
>> > +
>> > +       i++; /* one more entry for CPUFREQ_TABLE_END */
>> > +
>> > +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
>> > +       if (!new)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
>> > +               /* ignore invalid entry */
>> > +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
>> > +                       continue;
>> > +
>> > +               /* ignore duplicate entry */
>> > +               if (freq == old[i].frequency)
>> > +                       continue;
>> > +
>> > +               /* found an valid entry */
>> > +               new[level].frequency = old[i].frequency;
>> > +
>> > +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
>> > +                       descend = !!(freq > old[i].frequency);
>> > +
>> > +               /* freq always equals the last valid frequency */
>> > +               freq = new[level].frequency;
>> > +
>> > +               level++;
>> > +       }
>> > +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
>> > +
>> > +       /* convert to decending if in ascending order */
>> > +       if (!descend) {
>> > +               for(i = 0; i < (level - i - 1); i++) {
>> > +                       int j = level - i - 1;
>> > +                       freq = new[i].frequency;
>> > +                       new[i].frequency = new[j].frequency;
>> > +                       new[j].frequency = freq;
>> > +               }
>> > +       }
>> > +
>> > +       return new;
>> > +}
>> > +
>> >  /**
>> >   * get_cpu_frequency - get the absolute value of frequency from level.
>> >   * @cpu: cpu for which frequency is fetched.
>> > --
>> > 1.7.9.5
>> >
>
>

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[amit kachhap]

On Wed, Feb 6, 2013 at 6:53 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> On Wed, 2013-02-06 at 17:56 -0800, amit daniel kachhap wrote:
>> Hi Rui,
>>
>> I tested this code and after 2 changes it started working.
Oh sorry I didn't explained it . Actually I replied the problems to
their corresponding patches.
>>
> which two changes?
>
>> Also calling memory allocation for getting cpufreq table is quite
>> expensive and might also fail.
>> Storing the frequency again in a new
>> table can be avoided
>
> you mean it is not worth creating a cpu cooling table?
>
> do you mean this memory allocation is quite expensive for SOC?
> Sorry I do not have too much experience on SOC.
>
>>  and only required frequency or level can be
>> returned.
>>
> Sorry, I do not understand.
> But it is true that we do not have to allocate a new table to get the
> level/frequency.

I meant the same that it is unnecessary to allocate the cpufreq table
and then free them.

Thanks.
Amit Daniel
> I just want to make the cooling state <-> frequency transition clear and
> consistent for cpufreq cooling.
> I can work out another version without memory allocation, if that's what
> you want.

>
> thanks,
> rui
>> Thanks,
>> Amit Daniel
>>
>> On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote:
>> > CPU cooling table is a cleaned cpufreq_frequency_table that
>> > 1. each entry represents a cooling state,
>> >    aka, each entry has a different frequency.
>> > 2. does not have invalid entry.
>> > 3. does not have duplicate entry.
>> > 4. the frequencies of all the entries are in descending order.
>> >
>> > we should use this table inside thermal layer and thermal drivers.
>> >
>> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> > ---
>> >  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 63 insertions(+)
>> >
>> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> > index 455c77a..08f12c7 100644
>> > --- a/drivers/thermal/cpu_cooling.c
>> > +++ b/drivers/thermal/cpu_cooling.c
>> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
>> >         return !cpufreq_get_policy(&policy, cpu);
>> >  }
>> >
>> > +/*
>> > + * get the cleaned cpufreq_frequency_table that
>> > + * 1) does not have invalid entries
>> > + * 2) does not have duplicate entries
>> > + * 3) the frequency of the entries are in descending order
>> > + */
>> > +static struct cpufreq_frequency_table *
>> > +get_cpu_cooling_table(unsigned int cpu)
>> > +{
>> > +       int i = 0;
>> > +       int level;
>> > +       struct cpufreq_frequency_table *old, *new;
>> > +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
>> > +       int descend = -1;
>> > +
>> > +       old = cpufreq_frequency_get_table(cpu);
>> > +       if (!old)
>> > +               return ERR_PTR(-EINVAL);
>> > +
>> > +       while (old[i].frequency != CPUFREQ_TABLE_END)
>> > +               i++;
>> > +
>> > +       i++; /* one more entry for CPUFREQ_TABLE_END */
>> > +
>> > +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
>> > +       if (!new)
>> > +               return ERR_PTR(-ENOMEM);
>> > +
>> > +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
>> > +               /* ignore invalid entry */
>> > +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
>> > +                       continue;
>> > +
>> > +               /* ignore duplicate entry */
>> > +               if (freq == old[i].frequency)
>> > +                       continue;
>> > +
>> > +               /* found an valid entry */
>> > +               new[level].frequency = old[i].frequency;
>> > +
>> > +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
>> > +                       descend = !!(freq > old[i].frequency);
>> > +
>> > +               /* freq always equals the last valid frequency */
>> > +               freq = new[level].frequency;
>> > +
>> > +               level++;
>> > +       }
>> > +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
>> > +
>> > +       /* convert to decending if in ascending order */
>> > +       if (!descend) {
>> > +               for(i = 0; i < (level - i - 1); i++) {
>> > +                       int j = level - i - 1;
>> > +                       freq = new[i].frequency;
>> > +                       new[i].frequency = new[j].frequency;
>> > +                       new[j].frequency = freq;
>> > +               }
>> > +       }
>> > +
>> > +       return new;
>> > +}
>> > +
>> >  /**
>> >   * get_cpu_frequency - get the absolute value of frequency from level.
>> >   * @cpu: cpu for which frequency is fetched.
>> > --
>> > 1.7.9.5
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[Zhang Rui]

On Wed, 2013-02-06 at 17:56 -0800, amit daniel kachhap wrote:
> Hi Rui,
> 
> I tested this code and after 2 changes it started working.
> 
which two changes?

> Also calling memory allocation for getting cpufreq table is quite
> expensive and might also fail.
> Storing the frequency again in a new
> table can be avoided

you mean it is not worth creating a cpu cooling table?

do you mean this memory allocation is quite expensive for SOC?
Sorry I do not have too much experience on SOC. 

>  and only required frequency or level can be
> returned.
> 
Sorry, I do not understand.
But it is true that we do not have to allocate a new table to get the
level/frequency.
I just want to make the cooling state <-> frequency transition clear and
consistent for cpufreq cooling.
I can work out another version without memory allocation, if that's what
you want.

thanks,
rui
> Thanks,
> Amit Daniel
> 
> On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote:
> > CPU cooling table is a cleaned cpufreq_frequency_table that
> > 1. each entry represents a cooling state,
> >    aka, each entry has a different frequency.
> > 2. does not have invalid entry.
> > 3. does not have duplicate entry.
> > 4. the frequencies of all the entries are in descending order.
> >
> > we should use this table inside thermal layer and thermal drivers.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> > index 455c77a..08f12c7 100644
> > --- a/drivers/thermal/cpu_cooling.c
> > +++ b/drivers/thermal/cpu_cooling.c
> > @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
> >         return !cpufreq_get_policy(&policy, cpu);
> >  }
> >
> > +/*
> > + * get the cleaned cpufreq_frequency_table that
> > + * 1) does not have invalid entries
> > + * 2) does not have duplicate entries
> > + * 3) the frequency of the entries are in descending order
> > + */
> > +static struct cpufreq_frequency_table *
> > +get_cpu_cooling_table(unsigned int cpu)
> > +{
> > +       int i = 0;
> > +       int level;
> > +       struct cpufreq_frequency_table *old, *new;
> > +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
> > +       int descend = -1;
> > +
> > +       old = cpufreq_frequency_get_table(cpu);
> > +       if (!old)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       while (old[i].frequency != CPUFREQ_TABLE_END)
> > +               i++;
> > +
> > +       i++; /* one more entry for CPUFREQ_TABLE_END */
> > +
> > +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
> > +       if (!new)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
> > +               /* ignore invalid entry */
> > +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
> > +                       continue;
> > +
> > +               /* ignore duplicate entry */
> > +               if (freq == old[i].frequency)
> > +                       continue;
> > +
> > +               /* found an valid entry */
> > +               new[level].frequency = old[i].frequency;
> > +
> > +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
> > +                       descend = !!(freq > old[i].frequency);
> > +
> > +               /* freq always equals the last valid frequency */
> > +               freq = new[level].frequency;
> > +
> > +               level++;
> > +       }
> > +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
> > +
> > +       /* convert to decending if in ascending order */
> > +       if (!descend) {
> > +               for(i = 0; i < (level - i - 1); i++) {
> > +                       int j = level - i - 1;
> > +                       freq = new[i].frequency;
> > +                       new[i].frequency = new[j].frequency;
> > +                       new[j].frequency = freq;
> > +               }
> > +       }
> > +
> > +       return new;
> > +}
> > +
> >  /**
> >   * get_cpu_frequency - get the absolute value of frequency from level.
> >   * @cpu: cpu for which frequency is fetched.
> > --
> > 1.7.9.5
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] Thermal: Introduce cpu cooling table

[amit daniel kachhap]

Hi Rui,

I tested this code and after 2 changes it started working.

Also calling memory allocation for getting cpufreq table is quite
expensive and might also fail. Storing the frequency again in a new
table can be avoided and only required frequency or level can be
returned.

Thanks,
Amit Daniel

On Wed, Feb 6, 2013 at 5:46 PM, amit kachhap <amit.kachhap@gmail.com> wrote:
> CPU cooling table is a cleaned cpufreq_frequency_table that
> 1. each entry represents a cooling state,
>    aka, each entry has a different frequency.
> 2. does not have invalid entry.
> 3. does not have duplicate entry.
> 4. the frequencies of all the entries are in descending order.
>
> we should use this table inside thermal layer and thermal drivers.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/thermal/cpu_cooling.c |   63 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 455c77a..08f12c7 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -115,6 +115,69 @@ static int is_cpufreq_valid(int cpu)
>         return !cpufreq_get_policy(&policy, cpu);
>  }
>
> +/*
> + * get the cleaned cpufreq_frequency_table that
> + * 1) does not have invalid entries
> + * 2) does not have duplicate entries
> + * 3) the frequency of the entries are in descending order
> + */
> +static struct cpufreq_frequency_table *
> +get_cpu_cooling_table(unsigned int cpu)
> +{
> +       int i = 0;
> +       int level;
> +       struct cpufreq_frequency_table *old, *new;
> +       unsigned int freq = CPUFREQ_ENTRY_INVALID;
> +       int descend = -1;
> +
> +       old = cpufreq_frequency_get_table(cpu);
> +       if (!old)
> +               return ERR_PTR(-EINVAL);
> +
> +       while (old[i].frequency != CPUFREQ_TABLE_END)
> +               i++;
> +
> +       i++; /* one more entry for CPUFREQ_TABLE_END */
> +
> +       new = kzalloc(i * sizeof(struct cpufreq_frequency_table), GFP_KERNEL);
> +       if (!new)
> +               return ERR_PTR(-ENOMEM);
> +
> +       for (i = 0, level = 0; old[i].frequency != CPUFREQ_TABLE_END; i++) {
> +               /* ignore invalid entry */
> +               if (old[i].frequency == CPUFREQ_ENTRY_INVALID)
> +                       continue;
> +
> +               /* ignore duplicate entry */
> +               if (freq == old[i].frequency)
> +                       continue;
> +
> +               /* found an valid entry */
> +               new[level].frequency = old[i].frequency;
> +
> +               if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)
> +                       descend = !!(freq > old[i].frequency);
> +
> +               /* freq always equals the last valid frequency */
> +               freq = new[level].frequency;
> +
> +               level++;
> +       }
> +       new[level].frequency = CPUFREQ_ENTRY_INVALID;
> +
> +       /* convert to decending if in ascending order */
> +       if (!descend) {
> +               for(i = 0; i < (level - i - 1); i++) {
> +                       int j = level - i - 1;
> +                       freq = new[i].frequency;
> +                       new[i].frequency = new[j].frequency;
> +                       new[j].frequency = freq;
> +               }
> +       }
> +
> +       return new;
> +}
> +
>  /**
>   * get_cpu_frequency - get the absolute value of frequency from level.
>   * @cpu: cpu for which frequency is fetched.
> --
> 1.7.9.5