Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 04/02/2014 11:36 AM, Daniel Lezcano wrote:

> On 04/01/2014 10:33 AM, Dongsheng Wang wrote:
>> From: Wang Dongsheng <dongsheng.wang@freescale.com>
>>
>> Add cpuidle support for e500 family, using cpuidle framework to
>> manage various low power modes. The new implementation will remain
>> compatible with original idle method.
>>
>> I have done test about power consumption and latency. Cpuidle framework
>> will make CPU response time faster than original method, but power
>> consumption is higher than original method.
>>
>> Power consumption:
>> The original method, power consumption is 10.51202 (W).
>> The cpuidle framework, power consumption is 10.5311 (W).
>>
>> Latency:
>> The original method, avg latency is 6782 (us).
>> The cpuidle framework, avg latency is 6482 (us).
>>
>> Initially, this supports PW10, PW20 and subsequent patches will support
>> DOZE/NAP and PH10, PH20.
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>

Please fix Rafael's email when resending/answering.

Thanks

   -- Daniel

>> diff --git a/arch/powerpc/include/asm/machdep.h
>> b/arch/powerpc/include/asm/machdep.h
>> index 5b6c03f..9301420 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -294,6 +294,15 @@ extern void power7_idle(void);
>>   extern void ppc6xx_idle(void);
>>   extern void book3e_idle(void);
>>
>> +static inline void cpuidle_wait(void)
>> +{
>> +#ifdef CONFIG_PPC64
>> +    book3e_idle();
>> +#else
>> +    e500_idle();
>> +#endif
>> +}
>> +
>>   /*
>>    * ppc_md contains a copy of the machine description structure for the
>>    * current platform. machine_id contains the initial address where the
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 97e1dc9..edd193f 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device
>> *dev,
>>       return sprintf(buf, "%llu\n", time > 0 ? time : 0);
>>   }
>>
>> +#ifdef CONFIG_CPU_IDLE_E500
>> +u32 cpuidle_entry_bit;
>> +#endif
>>   static void set_pw20_wait_entry_bit(void *val)
>>   {
>>       u32 *value = val;
>> @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val)
>>       /* set count */
>>       pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT);
>>
>> +#ifdef CONFIG_CPU_IDLE_E500
>> +    cpuidle_entry_bit = *value;
>> +#else
>>       mtspr(SPRN_PWRMGTCR0, pw20_idle);
>> +#endif
>>   }
>>
>>   static ssize_t store_pw20_wait_time(struct device *dev,
>> diff --git a/drivers/cpuidle/Kconfig.powerpc
>> b/drivers/cpuidle/Kconfig.powerpc
>> index 66c3a09..0949dbf 100644
>> --- a/drivers/cpuidle/Kconfig.powerpc
>> +++ b/drivers/cpuidle/Kconfig.powerpc
>> @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE
>>       help
>>         Select this option to enable processor idle state management
>>         through cpuidle subsystem.
>> +
>> +config CPU_IDLE_E500
>> +    bool "CPU Idle Driver for E500 family processors"
>> +    depends on CPU_IDLE
>> +    depends on FSL_SOC_BOOKE
>> +    help
>> +      Select this to enable cpuidle on e500 family processors.
>> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>> index f71ae1b..7e6adea 100644
>> --- a/drivers/cpuidle/Makefile
>> +++ b/drivers/cpuidle/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE)          +=
>> cpuidle-at91.o
>>   # POWERPC drivers
>>   obj-$(CONFIG_PSERIES_CPUIDLE)        += cpuidle-pseries.o
>>   obj-$(CONFIG_POWERNV_CPUIDLE)        += cpuidle-powernv.o
>> +obj-$(CONFIG_CPU_IDLE_E500)        += cpuidle-e500.o
>> diff --git a/drivers/cpuidle/cpuidle-e500.c
>> b/drivers/cpuidle/cpuidle-e500.c
>> new file mode 100644
>> index 0000000..ddc0def
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-e500.c
>> @@ -0,0 +1,194 @@
>> +/*
>> + * CPU Idle driver for Freescale PowerPC e500 family processors.
>> + *
>> + * Copyright 2014 Freescale Semiconductor, Inc.
>> + *
>> + * Author: Dongsheng Wang <dongsheng.wang@freescale.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/cpuidle.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/notifier.h>
>> +
>> +#include <asm/cputable.h>
>> +#include <asm/machdep.h>
>> +#include <asm/mpc85xx.h>
>> +
>> +static unsigned int max_idle_state;
>> +static struct cpuidle_state *cpuidle_state_table;
>> +
>> +struct cpuidle_driver e500_idle_driver = {
>> +    .name = "e500_idle",
>> +    .owner = THIS_MODULE,
>> +};
>> +
>> +static void e500_cpuidle(void)
>> +{
>> +    if (cpuidle_idle_call())
>> +        cpuidle_wait();
>> +}
>
> Nope, that has been changed. No more call to cpuidle_idle_call in a driver.
>
>> +
>> +static int pw10_enter(struct cpuidle_device *dev,
>> +            struct cpuidle_driver *drv, int index)
>> +{
>> +    cpuidle_wait();
>> +    return index;
>> +}
>> +
>> +#define MAX_BIT    63
>> +#define MIN_BIT    1
>> +extern u32 cpuidle_entry_bit;
>> +static int pw20_enter(struct cpuidle_device *dev,
>> +        struct cpuidle_driver *drv, int index)
>> +{
>> +    u32 pw20_idle;
>> +    u32 entry_bit;
>> +    pw20_idle = mfspr(SPRN_PWRMGTCR0);
>> +    if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) {
>> +        pw20_idle &= ~PWRMGTCR0_PW20_ENT;
>> +        entry_bit = MAX_BIT - cpuidle_entry_bit;
>> +        pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT);
>> +        mtspr(SPRN_PWRMGTCR0, pw20_idle);
>> +    }
>> +
>> +    cpuidle_wait();
>> +
>> +    pw20_idle &= ~PWRMGTCR0_PW20_ENT;
>> +    pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT);
>> +    mtspr(SPRN_PWRMGTCR0, pw20_idle);
>> +
>> +    return index;
>> +}
>
> Is it possible to give some comments and encapsulate the code with
> explicit function names to be implemented in an arch specific directory
> file (eg. pm.c) and export these functions in a linux/ header ? We try
> to prevent to include asm if possible.
>
>> +
>> +static struct cpuidle_state pw_idle_states[] = {
>> +    {
>> +        .name = "pw10",
>> +        .desc = "pw10",
>> +        .flags = CPUIDLE_FLAG_TIME_VALID,
>> +        .exit_latency = 0,
>> +        .target_residency = 0,
>> +        .enter = &pw10_enter
>> +    },
>> +
>> +    {
>> +        .name = "pw20",
>> +        .desc = "pw20-core-idle",
>> +        .flags = CPUIDLE_FLAG_TIME_VALID,
>> +        .exit_latency = 1,
>> +        .target_residency = 50,
>> +        .enter = &pw20_enter
>> +    },
>> +};
>
> No need to define this intermediate structure here, you can directly
> initialize the cpuidle_driver:
>
>
> struct cpuidle_driver e500_idle_driver = {
>      .name = "e500_idle",
>      .owner = THIS_MODULE,
>      .states = {
>          .name = "pw10",
>          .desc = "pw10",
>          .flags = CPUIDLE_FLAG_TIME_VALID,
>          .target_residency = 0,
>          .enter = &pw10_enter,
>      },
>
>      ....
>
>      .state_count = 2,
> };
>
> Then in the init function you initialize the state_count consequently:
>
> if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
>      drv->state_count = 1;
>
> Then you can kill:
>
> max_idle_state, cpuidle_state_table, e500_idle_state_probe and
> pw_idle_states.
>
>> +
>> +static int cpu_hotplug_notify(struct notifier_block *n,
>> +            unsigned long action, void *hcpu)
>> +{
>> +    unsigned long hotcpu = (unsigned long)hcpu;
>> +    struct cpuidle_device *dev =
>> +            per_cpu_ptr(cpuidle_devices, hotcpu);
>> +
>> +    if (dev && cpuidle_get_driver()) {
>> +        switch (action) {
>> +        case CPU_ONLINE:
>> +        case CPU_ONLINE_FROZEN:
>> +            cpuidle_pause_and_lock();
>> +            cpuidle_enable_device(dev);
>> +            cpuidle_resume_and_unlock();
>> +            break;
>> +
>> +        case CPU_DEAD:
>> +        case CPU_DEAD_FROZEN:
>> +            cpuidle_pause_and_lock();
>> +            cpuidle_disable_device(dev);
>> +            cpuidle_resume_and_unlock();
>> +            break;
>> +
>> +        default:
>> +            return NOTIFY_DONE;
>> +        }
>> +    }
>> +
>> +    return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block cpu_hotplug_notifier = {
>> +    .notifier_call = cpu_hotplug_notify,
>> +};
>
> Can you explain why this is needed ?
>
>> +static void e500_cpuidle_driver_init(void)
>> +{
>> +    int idle_state;
>> +    struct cpuidle_driver *drv = &e500_idle_driver;
>
> Pass the cpuidle_driver as parameter to the function.
>
>> +
>> +    drv->state_count = 0;
>> +
>> +    for (idle_state = 0; idle_state < max_idle_state; ++idle_state) {
>> +        if (!cpuidle_state_table[idle_state].enter)
>> +            break;
>> +
>> +        drv->states[drv->state_count] = cpuidle_state_table[idle_state];
>> +        drv->state_count++;
>> +    }
>
> This code should disappear.
>
> As this function will just initialize state_count, you can move it in
> caller and kill this function.
>
>> +}
>> +
>> +static int e500_idle_state_probe(void)
>> +{
>> +    if (cpuidle_disable != IDLE_NO_OVERRIDE)
>> +        return -ENODEV;
>> +
>> +    cpuidle_state_table = pw_idle_states;
>> +    max_idle_state = ARRAY_SIZE(pw_idle_states);
>> +
>> +    /* Disable PW20 feature for e500mc, e5500 */
>> +    if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500)
>> +        cpuidle_state_table[1].enter = NULL;
>> +
>> +    if (!cpuidle_state_table || !max_idle_state)
>> +        return -ENODEV;
>> +
>> +    return 0;
>> +}
>
> This code should disappear.
>
>> +static void replace_orig_idle(void *dummy)
>> +{
>> +    return;
>> +}
>> +
>> +static int __init e500_idle_init(void)
>> +{
>> +    struct cpuidle_driver *drv = &e500_idle_driver;
>> +    int err;
>> +
>> +    if (e500_idle_state_probe())
>> +        return -ENODEV;
>> +
>> +    e500_cpuidle_driver_init();
>> +    if (!drv->state_count)
>> +        return -ENODEV;
>
> No need of this check, because:
>
> 1. you know how you initialized the driver (1 or 2 states)
> 2. this is already by the cpuidle framework
>
>> +
>> +    err = cpuidle_register(drv, NULL);
>> +    if (err) {
>> +        pr_err("Register e500 family cpuidle driver failed.\n");
>
> extra carriage return.
>> +
>> +        return err;
>> +    }
>> +
>> +    err = register_cpu_notifier(&cpu_hotplug_notifier);
>> +    if (err)
>> +        pr_warn("Cpuidle driver: register cpu notifier failed.\n");
>> +
>> +    /* Replace the original way of idle after cpuidle registered. */
>> +    ppc_md.power_save = e500_cpuidle;
>> +    on_each_cpu(replace_orig_idle, NULL, 1);
>
> Why ?
>
>> +    pr_info("e500_idle_driver registered.\n");
>> +
>> +    return 0;
>> +}
>> +late_initcall(e500_idle_init);
>>
>
> Thanks
>
>    -- Daniel
>
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev