Re: [linux-pm] [RFC PATCH 5/5] ACPI: thermal: processor: Use the generic cpufreq infrastructure

[amit daniel kachhap]

On Fri, May 16, 2014 at 12:36 AM, Eduardo Valentin <edubezval@xxxxxxxxx> wrote:
> Hello Amit,
>
> On Thu, May 08, 2014 at 08:08:00PM +0530, Amit Daniel Kachhap wrote:
>> This patch upgrades the ACPI cpufreq cooling portions to use the generic
>> cpufreq cooling infrastructure. There should not be any functionality
>> related changes as the same behaviour is provided by the generic
>> cpufreq APIs with the notifier mechanism.
>>
>
> This is a very good move as we are converging towards a more and more
> common infrastructure. How did you test this code?
Thanks.
Actually with linaro acpi kernel I was able to enable acpi processor
in my arndale(exynos5250) board. After that in some hacky way I
enabled acpi __TMP method to read its tmu temperatures and able to
test thermal cpufreq scaling. Ofcourse i am not able to test
cputhrottling as it is not supported.

>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
>> ---
>>  drivers/acpi/processor_driver.c  |    6 +-
>>  drivers/acpi/processor_thermal.c |  210 +++++++++++++++++---------------------
>>  2 files changed, 99 insertions(+), 117 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 7f70f31..10aba4a 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/cpuidle.h>
>>  #include <linux/slab.h>
>>  #include <linux/acpi.h>
>> +#include <linux/cpu_cooling.h>
>>
>>  #include <acpi/processor.h>
>>
>> @@ -178,8 +179,7 @@ static int __acpi_processor_start(struct acpi_device *device)
>>       if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>>               acpi_processor_power_init(pr);
>>
>> -     pr->cdev = thermal_cooling_device_register("Processor", device,
>> -                                                &processor_cooling_ops);
>> +     pr->cdev = acpi_processor_cooling_register(device);
>>       if (IS_ERR(pr->cdev)) {
>>               result = PTR_ERR(pr->cdev);
>>               goto err_power_exit;
>> @@ -250,7 +250,7 @@ static int acpi_processor_stop(struct device *dev)
>>       if (pr->cdev) {
>>               sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
>>               sysfs_remove_link(&pr->cdev->device.kobj, "device");
>> -             thermal_cooling_device_unregister(pr->cdev);
>> +             cpufreq_cooling_unregister(pr->cdev);
>>               pr->cdev = NULL;
>>       }
>>       return 0;
>> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
>> index e003663..c442a58 100644
>> --- a/drivers/acpi/processor_thermal.c
>> +++ b/drivers/acpi/processor_thermal.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/init.h>
>>  #include <linux/cpufreq.h>
>>  #include <linux/acpi.h>
>> +#include <linux/cpu_cooling.h>
>>  #include <acpi/processor.h>
>>  #include <asm/uaccess.h>
>>
>> @@ -53,28 +54,11 @@ ACPI_MODULE_NAME("processor_thermal");
>>
>>  static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>>  static unsigned int acpi_thermal_cpufreq_is_init = 0;
>> +static struct notifier_block cpufreq_cooling_notifier_block;
>>
>>  #define reduction_pctg(cpu) \
>>       per_cpu(cpufreq_thermal_reduction_pctg, phys_package_first_cpu(cpu))
>>
>> -/*
>> - * Emulate "per package data" using per cpu data (which should really be
>> - * provided elsewhere)
>> - *
>> - * Note we can lose a CPU on cpu hotunplug, in this case we forget the state
>> - * temporarily. Fortunately that's not a big issue here (I hope)
>> - */
>> -static int phys_package_first_cpu(int cpu)
>> -{
>> -     int i;
>> -     int id = topology_physical_package_id(cpu);
>> -
>> -     for_each_online_cpu(i)
>> -             if (topology_physical_package_id(i) == id)
>> -                     return i;
>> -     return 0;
>> -}
>> -
>>  static int cpu_has_cpufreq(unsigned int cpu)
>>  {
>>       struct cpufreq_policy policy;
>> @@ -83,30 +67,6 @@ static int cpu_has_cpufreq(unsigned int cpu)
>>       return 1;
>>  }
>>
>> -static int acpi_thermal_cpufreq_notifier(struct notifier_block *nb,
>> -                                      unsigned long event, void *data)
>> -{
>> -     struct cpufreq_policy *policy = data;
>> -     unsigned long max_freq = 0;
>> -
>> -     if (event != CPUFREQ_ADJUST)
>> -             goto out;
>> -
>> -     max_freq = (
>> -         policy->cpuinfo.max_freq *
>> -         (100 - reduction_pctg(policy->cpu) * 20)
>> -     ) / 100;
>> -
>> -     cpufreq_verify_within_limits(policy, 0, max_freq);
>> -
>> -      out:
>> -     return 0;
>> -}
>> -
>> -static struct notifier_block acpi_thermal_cpufreq_notifier_block = {
>> -     .notifier_call = acpi_thermal_cpufreq_notifier,
>> -};
>> -
>>  static int cpufreq_get_max_state(unsigned int cpu)
>>  {
>>       if (!cpu_has_cpufreq(cpu))
>> @@ -123,34 +83,32 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>>       return reduction_pctg(cpu);
>>  }
>>
>> -static int cpufreq_set_cur_state(unsigned int cpu, int state)
>> +static int acpi_processor_freq_level(unsigned int cpu, int state)
>>  {
>> -     int i;
>> +     struct cpufreq_policy policy;
>> +     unsigned long max_freq = 0;
>> +     int level = 0;
>>
>> -     if (!cpu_has_cpufreq(cpu))
>> +     if (!acpi_thermal_cpufreq_is_init || cpufreq_get_policy(&policy, cpu))
>>               return 0;
>>
>>       reduction_pctg(cpu) = state;
>> +     max_freq = (
>> +         policy.cpuinfo.max_freq *
>> +         (100 - reduction_pctg(cpu) * 20)
>> +     ) / 100;
>>
>> -     /*
>> -      * Update all the CPUs in the same package because they all
>> -      * contribute to the temperature and often share the same
>> -      * frequency.
>> -      */
>> -     for_each_online_cpu(i) {
>> -             if (topology_physical_package_id(i) ==
>> -                 topology_physical_package_id(cpu))
>> -                     cpufreq_update_policy(i);
>> -     }
>> -     return 0;
>> +     level =  cpufreq_cooling_get_level(phys_package_first_cpu(cpu),
>> +                                             max_freq, GET_LEVEL_FLOOR);
>> +     return level;
>>  }
>>
>>  void acpi_thermal_cpufreq_init(void)
>>  {
>>       int i;
>>
>> -     i = cpufreq_register_notifier(&acpi_thermal_cpufreq_notifier_block,
>> -                                   CPUFREQ_POLICY_NOTIFIER);
>> +     i = cpufreq_cooling_register_notifier(&cpufreq_cooling_notifier_block,
>> +                                     CPU_COOLING_STATE_NOTIFIER);
>>       if (!i)
>>               acpi_thermal_cpufreq_is_init = 1;
>>  }
>> @@ -158,9 +116,9 @@ void acpi_thermal_cpufreq_init(void)
>>  void acpi_thermal_cpufreq_exit(void)
>>  {
>>       if (acpi_thermal_cpufreq_is_init)
>> -             cpufreq_unregister_notifier
>> -                 (&acpi_thermal_cpufreq_notifier_block,
>> -                  CPUFREQ_POLICY_NOTIFIER);
>> +             cpufreq_cooling_unregister_notifier(
>> +                                             &cpufreq_cooling_notifier_block,
>> +                                             CPU_COOLING_STATE_NOTIFIER);
>>
>>       acpi_thermal_cpufreq_is_init = 0;
>>  }
>> @@ -176,13 +134,31 @@ static int cpufreq_get_cur_state(unsigned int cpu)
>>       return 0;
>>  }
>>
>> -static int cpufreq_set_cur_state(unsigned int cpu, int state)
>> +static int acpi_processor_freq_level(unsigned int cpu, int state)
>>  {
>>       return 0;
>>  }
>>
>>  #endif
>>
>> +/*
>> + * Emulate "per package data" using per cpu data (which should really be
>> + * provided elsewhere)
>> + *
>> + * Note we can lose a CPU on cpu hotunplug, in this case we forget the state
>> + * temporarily. Fortunately that's not a big issue here (I hope)
>> + */
>> +static int phys_package_first_cpu(int cpu)
>> +{
>> +     int i;
>> +     int id = topology_physical_package_id(cpu);
>> +
>> +     for_each_online_cpu(i)
>> +             if (topology_physical_package_id(i) == id)
>> +                     return i;
>> +     return 0;
>> +}
>> +
>>  /* thermal cooling device callbacks */
>>  static int acpi_processor_max_state(struct acpi_processor *pr)
>>  {
>> @@ -198,57 +174,22 @@ static int acpi_processor_max_state(struct acpi_processor *pr)
>>
>>       return max_state;
>>  }
>> -static int
>> -processor_get_max_state(struct thermal_cooling_device *cdev,
>> -                     unsigned long *state)
>> -{
>> -     struct acpi_device *device = cdev->devdata;
>> -     struct acpi_processor *pr;
>> -
>> -     if (!device)
>> -             return -EINVAL;
>> -
>> -     pr = acpi_driver_data(device);
>> -     if (!pr)
>> -             return -EINVAL;
>>
>> -     *state = acpi_processor_max_state(pr);
>> -     return 0;
>> -}
>> -
>> -static int
>> -processor_get_cur_state(struct thermal_cooling_device *cdev,
>> -                     unsigned long *cur_state)
>> +static int acpi_processor_cur_state(struct acpi_processor *pr)
>>  {
>> -     struct acpi_device *device = cdev->devdata;
>> -     struct acpi_processor *pr;
>> -
>> -     if (!device)
>> -             return -EINVAL;
>> -
>> -     pr = acpi_driver_data(device);
>> -     if (!pr)
>> -             return -EINVAL;
>> -
>> -     *cur_state = cpufreq_get_cur_state(pr->id);
>> +     int cur_state = 0;
>> +     cur_state = cpufreq_get_cur_state(pr->id);
>>       if (pr->flags.throttling)
>> -             *cur_state += pr->throttling.state;
>> -     return 0;
>> +             cur_state += pr->throttling.state;
>> +     return cur_state;
>>  }
>>
>> -static int
>> -processor_set_cur_state(struct thermal_cooling_device *cdev,
>> -                     unsigned long state)
>> +static int acpi_processor_set_cur_state(struct acpi_processor *pr,
>> +             struct cpufreq_cooling_status *cooling, unsigned long event)
>>  {
>> -     struct acpi_device *device = cdev->devdata;
>> -     struct acpi_processor *pr;
>> -     int result = 0;
>> -     int max_pstate;
>> -
>> -     if (!device)
>> -             return -EINVAL;
>> +     int result = 0, level = 0;
>> +     int max_pstate, state = cooling->new_state;
>>
>> -     pr = acpi_driver_data(device);
>>       if (!pr)
>>               return -EINVAL;
>>
>> @@ -257,20 +198,61 @@ processor_set_cur_state(struct thermal_cooling_device *cdev,
>>       if (state > acpi_processor_max_state(pr))
>>               return -EINVAL;
>>
>> -     if (state <= max_pstate) {
>> +     if (state <= max_pstate && event == CPU_COOLING_SET_STATE_PRE) {
>>               if (pr->flags.throttling && pr->throttling.state)
>>                       result = acpi_processor_set_throttling(pr, 0, false);
>> -             cpufreq_set_cur_state(pr->id, state);
>> -     } else {
>> -             cpufreq_set_cur_state(pr->id, max_pstate);
>> +     } else if (state > max_pstate && event == CPU_COOLING_SET_STATE_POST) {
>>               result = acpi_processor_set_throttling(pr,
>>                               state - max_pstate, false);
>>       }
>> +
>> +     level = acpi_processor_freq_level(pr->id, state);
>> +     cooling->new_state = level;
>> +
>>       return result;
>>  }
>>
>> -const struct thermal_cooling_device_ops processor_cooling_ops = {
>> -     .get_max_state = processor_get_max_state,
>> -     .get_cur_state = processor_get_cur_state,
>> -     .set_cur_state = processor_set_cur_state,
>> +static int acpi_cpufreq_cooling_notifier(struct notifier_block *nb,
>> +                                      unsigned long event, void *data)
>> +{
>> +     struct cpufreq_cooling_status *cooling = data;
>> +     struct acpi_device *device = cooling->devdata;
>> +     struct acpi_processor *pr = acpi_driver_data(device);
>> +     switch (event) {
>> +     case CPU_COOLING_SET_STATE_PRE:
>> +     case CPU_COOLING_SET_STATE_POST:
>> +             acpi_processor_set_cur_state(pr, cooling, event);
>> +             break;
>> +     case CPU_COOLING_GET_MAX_STATE:
>> +             cooling->max_state = acpi_processor_max_state(pr);
>> +             break;
>> +     case CPU_COOLING_GET_CUR_STATE:
>> +             cooling->cur_state = acpi_processor_cur_state(pr);
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static struct notifier_block cpufreq_cooling_notifier_block = {
>> +     .notifier_call = acpi_cpufreq_cooling_notifier,
>>  };
>> +
>> +struct thermal_cooling_device *
>> +acpi_processor_cooling_register(struct acpi_device *device)
>> +{
>> +     struct thermal_cooling_device *cdev;
>> +     struct acpi_processor *pr = acpi_driver_data(device);
>> +     int cpu = phys_package_first_cpu(pr->id);
>> +     int i;
>> +     int id = topology_physical_package_id(cpu);
>> +     struct cpumask cpus;
>> +
>> +     for_each_online_cpu(i)
>> +             if (topology_physical_package_id(i) == id)
>> +                     cpumask_set_cpu(i, &cpus);
>> +
>> +     cdev = cpufreq_cooling_register(&cpus, (void *)device);
>> +     return cdev;
>> +}
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/