Re: [RFC PATCH v4 2/4] ARM: omap: Remove cpuidle timekeeping and irq enable/disable

[Jean Pihet]

Rob,

On Wed, Feb 1, 2012 at 4:00 AM, Robert Lee  wrote:
> Now that the core cpuidle driver keeps time and handles irq enabling,
> remove this functionality.  Also, remove irq disabling as all paths to
> cpuidle_idle_call already call local_irq_disable.  Also, restructure
> idle functions as needed by the cpuidle core driver changes.
>
> Signed-off-by: Robert Lee 
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   96 
>  1 files changed, 43 insertions(+), 53 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 464cffd..9ecded5 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
...

> @@ -100,23 +107,20 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>                                struct cpuidle_driver *drv,
>                                int index)
>  {
> -       struct omap3_idle_statedata *cx =
> -                       cpuidle_get_statedata(&dev->states_usage[index]);
> -       struct timespec ts_preidle, ts_postidle, ts_idle;
> -       u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
> -       int idle_time;
> +       struct omap3_idle_drvdata *dd = dev->states_usage[index].driver_data
A build error is triggered by the missing ";".

...

> @@ -147,18 +151,12 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>                pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
>        }
>
> -return_sleep_time:
> -       getnstimeofday(&ts_postidle);
> -       ts_idle = timespec_sub(ts_postidle, ts_preidle);
> -
> -       local_irq_enable();
> +leave:
>        local_fiq_enable();
>
> -       idle_time = ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * \
> -                                                               USEC_PER_SEC;
> -
> -       /* Update cpuidle counters */
> -       dev->last_residency = idle_time;
> +       /* Restore original PER state if it was modified */
> +       if (dd->per_next_state != dd->per_saved_state)
> +               pwrdm_set_next_pwrst(per_pd, dd->per_saved_state);
This code is not necessarily balanced with the PER state change in
omap3_enter_idle_bm (cf. "/* Are we changing PER target state? */"
here below), since in the core cpuidle code there is no guarantee that
the calls to pre_enter and enter callbacks are balanced.
In general I fear that splitting the code in two functions introduces
a risk of programming non-coherent settings in the PRCM.

...

> @@ -255,8 +255,9 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>                               int index)
>  {
>        int new_state_idx;
> -       u32 core_next_state, per_next_state = 0, per_saved_state = 0, 
> cam_state;
> -       struct omap3_idle_statedata *cx;
> +       u32 core_next_state, cam_state;
> +       struct omap3_idle_drvdata *dd = dev->states_usage[index].driver_data;
> +       struct omap3_idle_statedata *cx = &dd->state_data[index];
>        int ret;
The build throws a few warnings about unused variables:

arch/arm/mach-omap2/cpuidle34xx.c: In function 'omap3_enter_idle_bm':
arch/arm/mach-omap2/cpuidle34xx.c:261: warning: unused variable 'ret'
arch/arm/mach-omap2/cpuidle34xx.c:257: warning: unused variable 'new_state_idx'

...

>
>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
> @@ -337,7 +329,8 @@ static inline void _fill_cstate(struct cpuidle_driver 
> *drv,
>        state->exit_latency     = cpuidle_params_table[idx].exit_latency;
>        state->target_residency = cpuidle_params_table[idx].target_residency;
>        state->flags            = CPUIDLE_FLAG_TIME_VALID;
> -       state->enter            = omap3_enter_idle_bm;
> +       state->pre_enter        = omap3_enter_idle_bm;
> +       state->enter            = omap3_enter_idle;
>        sprintf(state->name, "C%d", idx + 1);
>        strncpy(state->desc, descr, CPUIDLE_DESC_LEN);
>
...

Also the line at 373 is not needed anymore in omap3_idle_init, since
the enter callback is filled in in the _fill_cstate function:
/* C1 . MPU WFI + Core active */
_fill_cstate(drv, 0, "MPU ON + CORE ON");
(&drv->states[0])->enter = omap3_enter_idle; <==
not needed anymore
drv->safe_state_index = 0;

More testing on OMAP3 is needed. Let me come back with the results asap.

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v5 1/9] cpuidle: Add commonly used functionality for consolidation

[Jean Pihet]

Robert,

On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee  wrote:
> Add functionality that is commonly duplicated by various platforms.
>
> Signed-off-by: Robert Lee 
> ---
>  drivers/cpuidle/cpuidle.c |   37 ++
>  include/linux/cpuidle.h   |   55 
> +
>  2 files changed, 77 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
...

> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..6563683 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define CPUIDLE_STATE_MAX      8
>  #define CPUIDLE_NAME_LEN       16
> @@ -56,6 +57,16 @@ struct cpuidle_state {
>
>  #define CPUIDLE_DRIVER_FLAGS_MASK (0x)
>
> +/* Common ARM WFI state */
> +#define CPUIDLE_ARM_WFI_STATE {\
> +       .enter                  = cpuidle_simple_enter,\
> +       .exit_latency           = 1,\
> +       .target_residency       = 1,\
> +       .flags                  = CPUIDLE_FLAG_TIME_VALID,\
> +       .name                   = "WFI",\
> +       .desc                   = "ARM core clock gating (WFI)",\
> +}
This macro should belong to an ARM only header.

> +
>  /**
>  * cpuidle_get_statedata - retrieves private driver state data
>  * @st_usage: the state usage statistics
> @@ -122,6 +133,14 @@ struct cpuidle_driver {
>        struct module           *owner;
>
>        unsigned int            power_specified:1;
> +       /*
> +        * use the core cpuidle time keeping. This has been implemented for 
> the
> +        * entire driver instead of per state as currently the device enter
> +        * fuction allows the entered state to change which requires handling
> +        * that requires a (subjectively) unnecessary decrease to efficiency
> +        * in this commonly called code.
> +        */
> +       unsigned int            en_core_tk_irqen:1;
Does the description reads as 'the time accounting is only performed
if en_core_tk_irqen is set'?
Also it suggests that changing (i.e. demoting) the state is kind of a
problem, which is unclear. IIUC the state change is handled correctly
in cpuidle_idle_call, is that correct?

>        struct cpuidle_state    states[CPUIDLE_STATE_MAX];
>        int                     state_count;
>        int                     safe_state_index;
> @@ -140,6 +159,39 @@ extern void cpuidle_pause_and_lock(void);
>  extern void cpuidle_resume_and_unlock(void);
>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
> +extern int cpuidle_simple_enter(struct cpuidle_device *dev,
> +               struct cpuidle_driver *drv, int index);
> +
> +/**
> + * cpuidle_enter_wrap - performing timekeeping and irq around enter function
> + * @dev: pointer to a valid cpuidle_device object
> + * @drv: pointer to a valid cpuidle_driver object
> + * @index: index of the target cpuidle state.
> + */
> +static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
> +                               struct cpuidle_driver *drv, int index,
> +                               int (*enter)(struct cpuidle_device *dev,
> +                                       struct cpuidle_driver *drv, int 
> index))
The function name does not match the description (cpuidle_enter_wrap
vs cpuidle_wrap_enter).

> +{
> +       ktime_t time_start, time_end;
> +       s64 diff;
> +
> +       time_start = ktime_get();
> +
> +       index = enter(dev, drv, index);
> +
> +       time_end = ktime_get();
> +
> +       local_irq_enable();
> +
> +       diff = ktime_to_us(ktime_sub(time_end, time_start));
> +       if (diff > INT_MAX)
> +               diff = INT_MAX;
> +
> +       dev->last_residency = (int) diff;
> +
> +       return index;
> +}
>
>  #else
>  static inline void disable_cpuidle(void) { }
...

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v5 3/9] ARM: omap: Consolidate OMAP3 cpuidle time keeping and irq enable

[Jean Pihet]

On Mon, Feb 27, 2012 at 5:47 AM, Robert Lee  wrote:
> Use core cpuidle timekeeping and irqen wrapper and remove that
> handling from this code.
>
> Signed-off-by: Robert Lee 
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   43 +++-
>  1 files changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 464cffd..1f6123f 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
...

>  /**
> + * omap3_enter_idle - Programs OMAP3 to enter the specified state
> + * @dev: cpuidle device
> + * @drv: cpuidle driver
> + * @index: the index of state to be entered
> + *
> + * Called from the CPUidle framework to program the device to the
> + * specified target state selected by the governor.
> + */
> +static int omap3_enter_idle(struct cpuidle_device *dev,
> +                               struct cpuidle_driver *drv,
> +                               int index)
> +{
> +       return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle);
Is it the intention to call cpuidle_wrap_enter from the non-common
code? Could the driver set en_core_tk_irqen to 1 and so let the core
call the idle function? Is it to have the time measurement code closer
to the low level idle code in  __omap3_enter_idle?

> +}
> +
> +/**
>  * next_valid_state - Find next valid C-state
>  * @dev: cpuidle device
>  * @drv: cpuidle driver
> @@ -295,6 +287,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>        new_state_idx = next_valid_state(dev, drv, index);
>
>  select_state:
> +
Stray change

>        ret = omap3_enter_idle(dev, drv, new_state_idx);
>
>        /* Restore original PER state if it was modified */
> --
> 1.7.1
>

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: anybody willing to boot a test kernel on an omap34xx/omap2 board for me?

[Jean Pihet]

Hi Peter,

On Mon, Feb 27, 2012 at 9:35 PM, Peter Maydell  wrote:
> Hi; I'm hoping somebody will be willing to run a test kernel
> for me on some omap boards and send me the dmesg output.
> (I'm trying to sort out QEMU's modelling of the OMAP ID
> registers and the TRMs are rather unhelpful; in particular
> the OMAP35x TRM claims that there are two overlapping registers
> at addresses 0x218-0x21c!)
>
> I've done a Beagle xM which is an OMAP3630, but I'm looking for
> the results for OMAP34xx/35xx (I think classic Beagle is this)
> and also if possible an OMAP2 board.
>
> The kernel is http://people.linaro.org/~pmaydell/uImage.test
> (it's the linaro 3.0 kernel with a stock omap2_defconfig and
> some extra printks so it shouldn't do anything too alarming
> to your board :-)). It needs the command line arguments
> 'earlyprintk debug console=ttyO2,115200'
> (possibly for omap2 the console tty device is different)
>
> and I'm interested in the bit of dmesg output near the start:
> [    0.00] PMM: ID register dump:
> [    0.00] register offset 0x204 value 0x0b89102f
> [    0.00] register offset 0x208 value 0x
> [    0.00] register offset 0x20c value 0x
> [    0.00] register offset 0x210 value 0x00f0
> [    0.00] register offset 0x214 value 0xcafeb891
> [    0.00] register offset 0x218 value 0x0703201f
> [    0.00] register offset 0x21c value 0x015739ea
> [    0.00] register offset 0x220 value 0x1bf0
> [    0.00] register offset 0x224 value 0x5dd4
> [    0.00] OMAP3630 ES1.0 (l2cache iva sgx neon isp 192mhz_clk )
>
> (so you don't need the initrd/modules/etc, it's ok if it
> doesn't boot fully.)

Here is the console output on a Beagleboard (3530 ES2.1):

[0.00] Linux version 3.0.12-00764-g1a49060-dirty
(petmay01@cam-vm-266) (gcc vers
ion 4.6.1 (Ubuntu/Linaro 4.6.1-7ubuntu2~ppa3) ) #3 SMP Mon Feb 27
19:59:45 UTC 2012
[0.00] CPU: ARMv7 Processor [411fc082] revision 2 (ARMv7), cr=10c53c7d
[0.00] CPU: VIPT nonaliasing data cache, VIPT nonaliasing
instruction cache
[0.00] Machine: OMAP3 Beagle Board
[0.00] Memory policy: ECC disabled, Data cache writeback
[0.00] PMM: ID register dump:
[0.00] register offset 0x204 value 0x2b7ae02f
[0.00] register offset 0x208 value 0x
[0.00] register offset 0x20c value 0x
[0.00] register offset 0x210 value 0x00f0
[0.00] register offset 0x214 value 0xcafeb7ae
[0.00] register offset 0x218 value 0x09008009
[0.00] register offset 0x21c value 0x04013ef1
[0.00] register offset 0x220 value 0x
[0.00] register offset 0x224 value 0x0f62
[0.00] OMAP3430/3530 ES2.1 (l2cache iva sgx neon isp )
[0.00] SRAM: Mapped pa 0x4020 to va 0xfe40 size: 0x1
[0.00] On node 0 totalpages: 32768
[0.00] free_area_init_node: node 0, pgdat c0655920,
node_mem_map c0bac000
[0.00]   Normal zone: 256 pages used for memmap
[0.00]   Normal zone: 0 pages reserved
[0.00]   Normal zone: 32512 pages, LIFO batch:7
[0.00] Clocking rate (Crystal/Core/MPU): 26.0/332/500 MHz
[0.00] Reprogramming SDRC clock to 33200 Hz
[0.00] PERCPU: Embedded 7 pages/cpu @c0cb s5984 r8192 d14496 u32768
[0.00] pcpu-alloc: s5984 r8192 d14496 u32768 alloc=8*4096
[0.00] pcpu-alloc: [0] 0
[0.00] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 3251
2
[0.00] Kernel command line: earlyprintk debug console=ttyO2,115200
[0.00] PID hash table entries: 512 (order: -1, 2048 bytes)
[0.00] Dentry cache hash table entries: 16384 (order: 4, 65536 bytes)
[0.00] Inode-cache hash table entries: 8192 (order: 3, 32768 bytes)
[0.00] Memory: 128MB = 128MB total
[0.00] Memory: 117952k/117952k available, 13120k reserved, 0K highmem
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xfff0 - 0xfffe   ( 896 kB)
[0.00] DMA : 0xffc0 - 0xffe0   (   2 MB)
[0.00] vmalloc : 0xc880 - 0xf800   ( 760 MB)
[0.00] lowmem  : 0xc000 - 0xc800   ( 128 MB)
[0.00] modules : 0xbf00 - 0xc000   (  16 MB)
[0.00]   .text : 0xc0008000 - 0xc058c2c8   (5649 kB)
[0.00]   .init : 0xc058d000 - 0xc05d4760   ( 286 kB)
[0.00]   .data : 0xc05d6000 - 0xc0656d40   ( 516 kB)
[0.00].bss : 0xc0656d64 - 0xc0babd64   (5460 kB)
[0.00] Hierarchical RCU implementation.
[0.00] NR_IRQS:410
[0.00] IRQ: Found an INTC at 0xfa20 (revision 4.0) with 96
interrupts
[0.00] Total of 96 interrupts on 1 active controller
[0.00] omap_hwmod: gpt12_fck: missing clockdomain for gpt12_fck.
[0.00] OMAP clockevent source: GPTIMER12 at 32768 Hz
[0.

Re: [PATCH v6 1/9] cpuidle: Add common time keeping and irq enabling

[Jean Pihet]

Hi Rob,

On Wed, Feb 29, 2012 at 4:11 AM, Robert Lee  wrote:
> Make necessary changes to implement time keeping and irq enabling
> in the core cpuidle code.  This will allow the removal of these
> functionalities from various platform cpuidle implementations whose
> timekeeping and irq enabling follows the form in this common code.
>
> Signed-off-by: Robert Lee 
> ---
>  arch/arm/include/asm/cpuidle.h |   14 ++
>  drivers/cpuidle/cpuidle.c      |   90 
> 
>  include/linux/cpuidle.h        |   13 ++
>  3 files changed, 99 insertions(+), 18 deletions(-)
>  create mode 100644 arch/arm/include/asm/cpuidle.h
>
...

> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..00b02f5 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
...

> @@ -110,7 +126,8 @@ int cpuidle_idle_call(void)
>                dev->states_usage[entered_state].time +=
>                                (unsigned long long)dev->last_residency;
>                dev->states_usage[entered_state].usage++;
> -       }
> +       } else
> +               dev->last_residency = 0;
Braces are required here, according to the coding style doc.

...

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v6 0/9] Consolidate cpuidle functionality

[Jean Pihet]

Rob,

On Wed, Feb 29, 2012 at 4:11 AM, Robert Lee  wrote:
> This patch series moves various functionality duplicated in platform
> cpuidle drivers to the core cpuidle driver. Also, the platform irq
> disabling was removed as it appears that all calls into
> cpuidle_call_idle will have already called local_irq_disable().
>
> NOTE to Maintainers: Platform code changes are not required due to patch 1/9
> but please review and push these platform changes as possible to allow
> this consolidation to occur.
>
> Based on 3.3-rc5 plus recent exynos cpuidle patch (affect exynos cpuidle 
> only):
> http://www.spinics.net/lists/linux-samsung-soc/msg09467.html
>
> v5 submission can be found here:
> http://www.spinics.net/lists/arm-kernel/msg161596.html
> Changes since v5:
> * Fixed mindless bug in CONFIG_ARCH_HAS_RELAX code in 
> drivers/cpuidle/cpuidle.c
> * Removed inline from wrapper function (thanks Mike Turquette and Rob Herring)
> * Add zeroing out of last_residency if error value is returned (thanks Mike)
> * Made drivers/cpuidle/cpuidle.c more intelligently handle en_core_tk_irqen
> flag allowing removal of the if (en_core_tk_irqen) in cpuidle_idle_call 
> (thanks
> Daniel Lezcano)
> * Moved CPUIDLE_ARM_WFI_STATE macro to arch/arm/include/asm/cpuidle.h (thanks
> Jean Pihet)
> * Cleaned up some comments and a stray change (thanks Jean)

Except the comments I sent to the list, this version looks good:
Acked-by: Jean Pihet 

Tested OK using cpuidle in RETention and OFF modes on Beagleboard
(OMAP3530 ES2.1):
Tested-by: Jean Pihet 

Thanks & regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v6 5/9] ARM: davinci: Consolidate time keeping and irq enable

[Jean Pihet]

Rob,

On Wed, Feb 29, 2012 at 4:11 AM, Robert Lee  wrote:
> Enable core cpuidle timekeeping and irq enabling and remove that
> handling from this code.
>
> Signed-off-by: Robert Lee 
> ---
>  arch/arm/mach-davinci/cpuidle.c |   78 +++---
>  1 files changed, 31 insertions(+), 47 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
> index a30c7c5..6f457f1 100644
> --- a/arch/arm/mach-davinci/cpuidle.c
> +++ b/arch/arm/mach-davinci/cpuidle.c
...

> @@ -30,12 +31,42 @@ struct davinci_ops {
>        u32 flags;
>  };
>
> +/* Actual code that puts the SoC in different idle states */
> +static int davinci_enter_idle(struct cpuidle_device *dev,
> +                               struct cpuidle_driver *drv,
> +                                               int index)
> +{
> +       struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
> +       struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
> +
> +       if (ops && ops->enter)
> +               ops->enter(ops->flags);
> +
> +       return cpuidle_wrap_enter(dev,  drv, index,
> +                               cpuidle_simple_enter);
This does not look right since ops->exit will never be called.

> +
> +       if (ops && ops->exit)
> +               ops->exit(ops->flags);
> +
> +       return index;
> +}
> +
...

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Jean Pihet]

Hi Santosh, Daniel,

On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar
 wrote:
> Daniel,
>
> On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
>> This patchset is a proposition to improve a bit the code.
>> The changes are code cleanup and does not change the behavior of the
>> driver itself.
>>
>> A couple a things call my intention. Why the cpuidle device is set for cpu0 
>> only
>> and why the WFI is not used ?
>>
>> Daniel Lezcano (7):
>>   ARM: OMAP4: cpuidle - Remove unused valid field
>>   ARM: OMAP4: cpuidle - Declare the states with the driver declaration
>>   ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table
>>   ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
>>   ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time
>>   ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly
>>   ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot
>>     time
>>
> The series looks fine to me in general. This clean-up is applicable
> for OMAP3 cpuidle code as well.
>
> I want Jean to look at this series because some of his earlier
> clean up has introduced those custom functions which
> are getting removed in this series.
I am OK with the patch set, I only have minor remarks to the individual patches.

Reviewed-by: Jean Pihet 

>
> Regards
> santosh
>
>

Thanks!
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 1/7] ARM: OMAP4: cpuidle - Remove unused valid field

[Jean Pihet]

On Wed, Mar 21, 2012 at 11:03 AM, Santosh Shilimkar
 wrote:
> On Wednesday 21 March 2012 03:16 PM, Daniel Lezcano wrote:
>> On 03/21/2012 10:41 AM, Shilimkar, Santosh wrote:
>>> On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano
>>>   wrote:
 The 'valid' field is never used in the code, let's remove it.

 Signed-off-by: Daniel Lezcano
 ---
>>> It is used during the registration. This field has been very useful for
>>> debug when need to disable a C-state etc.
>>> So unless and until there is a strong reason, i would like to retain it.
>>
>> IMO if it used for debug purpose, it should be moved to the debug code
>> and if the debug code is not upstream, then that 'valid' should not be
>> here but in the out-of-tree code.
>>
> When I said debug, I mean CPUIDLE debug and not any special debug code.
>
>> By the way, this may be a debate for nothing because a patchset is on
>> the way to disable C-states from sysfs.
>>
> I see but sysfs won't solve that because you want to disable certain
> C-state so that CPUIDLE driver don't use that state.
This will solve the problem, the only difference is that you need the
user space to switch the disable knob from sysfs.
>From the kernel space, for debug, you can set the .disable value to 1
in the cpuidle_driver->states struct.

>
> Let say if the C4 which is OSWR is broken. In such cases, just
> setting valid flag let you disable it.
>
> Again I don't have strong objection to this change.
>
> Regards
> santosh

Regards,
Jean

>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 2/7] ARM: OMAP4: cpuidle - Declare the states with the driver declaration

[Jean Pihet]

On Wed, Mar 21, 2012 at 10:27 AM, Daniel Lezcano
 wrote:
> The cpuidle API allows to declare statically the states in the driver
> structure. Let's use it.
> We do no longer need the fill_cstate function called at runtime and
> by the way adding more instructions at boot time.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-omap2/cpuidle44xx.c |   56 
> +
>  1 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle44xx.c 
> b/arch/arm/mach-omap2/cpuidle44xx.c
> index 1210229..cd6bee7 100644
> --- a/arch/arm/mach-omap2/cpuidle44xx.c
> +++ b/arch/arm/mach-omap2/cpuidle44xx.c
> @@ -148,21 +148,38 @@ DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev);
>  struct cpuidle_driver omap4_idle_driver = {
>        .name =         "omap4_idle",
>        .owner =        THIS_MODULE,
> +       .states = {
> +               {
> +                       /* C1 - CPU0 ON + CPU1 ON + MPU ON */
> +                       .exit_latency = 2 + 2,
> +                       .target_residency = 5,
> +                       .flags = CPUIDLE_FLAG_TIME_VALID,
> +                       .enter = omap4_enter_idle,
> +                       .name = "C1",
> +                       .desc = "MPUSS ON"
> +               },
...
> +       },
> +       .state_count = OMAP4_NUM_STATES,
>  };
>
> -static inline void _fill_cstate(struct cpuidle_driver *drv,
> -                                       int idx, const char *descr)
> -{
> -       struct cpuidle_state *state = &drv->states[idx];
> -
> -       state->exit_latency     = cpuidle_params_table[idx].exit_latency;
> -       state->target_residency = cpuidle_params_table[idx].target_residency;
> -       state->flags            = CPUIDLE_FLAG_TIME_VALID;
> -       state->enter            = omap4_enter_idle;
> -       sprintf(state->name, "C%d", idx + 1);
> -       strncpy(state->desc, descr, CPUIDLE_DESC_LEN);
> -}
I am OK with this change, which makes the code more readable (and so
maintainable).

> -
>  static inline struct omap4_idle_statedata *_fill_cstate_usage(
>                                        struct cpuidle_device *dev,
>                                        int idx)
> @@ -196,37 +213,28 @@ int __init omap4_idle_init(void)
>        if ((!mpu_pd) || (!cpu0_pd) || (!cpu1_pd))
>                return -ENODEV;
>
> -
> -       drv->safe_state_index = -1;
>        dev = &per_cpu(omap4_idle_dev, cpu_id);
>        dev->cpu = cpu_id;
>
> -       /* C1 - CPU0 ON + CPU1 ON + MPU ON */
> -       _fill_cstate(drv, 0, "MPUSS ON");
> -       drv->safe_state_index = 0;
I would keep this or add a clear comment that C1 is the safe state.

...

Thanks,
Jean

> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Jean Pihet]

On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar
 wrote:
> Daniel,
>
> On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
>> This patchset is a proposition to improve a bit the code.
>> The changes are code cleanup and does not change the behavior of the
>> driver itself.
>>
>> A couple a things call my intention. Why the cpuidle device is set for cpu0 
>> only
>> and why the WFI is not used ?
>>
>> Daniel Lezcano (7):
>>   ARM: OMAP4: cpuidle - Remove unused valid field
>>   ARM: OMAP4: cpuidle - Declare the states with the driver declaration
>>   ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table
>>   ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
>>   ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time
>>   ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly
>>   ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot
>>     time
>>
> The series looks fine to me in general. This clean-up is applicable
> for OMAP3 cpuidle code as well.
Great!
However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
- the latency settings can be overriden by the board code, so the
cpuidle_params struct is needed.

> I want Jean to look at this series because some of his earlier
> clean up has introduced those custom functions which
> are getting removed in this series.
>
> Regards
> santosh
>
>

Thanks,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 2/9] ARM: OMAP3: cpuidle - remove next_valid_state function

[Jean Pihet]

Hi Daniel,

On Fri, Mar 23, 2012 at 10:26 AM, Daniel Lezcano
 wrote:
> As we will be able to remove C-states from userspace with the sysfs
> API, this function is no longer needed. We remove it and that simplifies
> the code for more consolidation.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   85 
> +
>  1 files changed, 2 insertions(+), 83 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index d67431a..65b4e7aa 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -164,84 +164,6 @@ return_sleep_time:
>  }
>
>  /**
> - * next_valid_state - Find next valid C-state
> - * @dev: cpuidle device
> - * @drv: cpuidle driver
> - * @index: Index of currently selected c-state
> - *
> - * If the state corresponding to index is valid, index is returned back
> - * to the caller. Else, this function searches for a lower c-state which is
> - * still valid (as defined in omap3_power_states[]) and returns its index.
> - *
> - * A state is valid if the 'valid' field is enabled and
> - * if it satisfies the enable_off_mode condition.
> - */
> -static int next_valid_state(struct cpuidle_device *dev,
> -                       struct cpuidle_driver *drv,
> -                               int index)
> -{
> -       struct cpuidle_state_usage *curr_usage = &dev->states_usage[index];
> -       struct cpuidle_state *curr = &drv->states[index];
> -       struct omap3_idle_statedata *cx = cpuidle_get_statedata(curr_usage);
> -       u32 mpu_deepest_state = PWRDM_POWER_RET;
> -       u32 core_deepest_state = PWRDM_POWER_RET;
> -       int next_index = -1;
> -
> -       if (enable_off_mode) {
> -               mpu_deepest_state = PWRDM_POWER_OFF;
> -               /*
> -                * Erratum i583: valable for ES rev < Es1.2 on 3630.
> -                * CORE OFF mode is not supported in a stable form, restrict
> -                * instead the CORE state to RET.
> -                */
> -               if (!IS_PM34XX_ERRATUM(PM_SDRC_WAKEUP_ERRATUM_i583))
> -                       core_deepest_state = PWRDM_POWER_OFF;
> -       }
I am OK to remove the function but some of the code needs to stay (can
be moved into enter_idle_bm).
The errata code is needed.

> -
> -       /* Check if current state is valid */
> -       if ((cx->valid) &&
> -           (cx->mpu_state >= mpu_deepest_state) &&
> -           (cx->core_state >= core_deepest_state)) {
> -               return index;
> -       } else {
> -               int idx = OMAP3_NUM_STATES - 1;
On OMAP3 not all the power states combinations are allowed, so this
code looks for a C-state which satisfies the enable_off_mode flag.
I am planning to remove the enable_off_mode flag but this has to come
with the per-device PM QoS constraints code.

> -
> -               /* Reach the current state starting at highest C-state */
> -               for (; idx >= 0; idx--) {
> -                       if (&drv->states[idx] == curr) {
> -                               next_index = idx;
> -                               break;
> -                       }
> -               }
> -
> -               /* Should never hit this condition */
> -               WARN_ON(next_index == -1);
> -
> -               /*
> -                * Drop to next valid state.
> -                * Start search from the next (lower) state.
> -                */
> -               idx--;
> -               for (; idx >= 0; idx--) {
> -                       cx = cpuidle_get_statedata(&dev->states_usage[idx]);
> -                       if ((cx->valid) &&
> -                           (cx->mpu_state >= mpu_deepest_state) &&
> -                           (cx->core_state >= core_deepest_state)) {
> -                               next_index = idx;
> -                               break;
> -                       }
> -               }
> -               /*
> -                * C1 is always valid.
> -                * So, no need to check for 'next_index == -1' outside
> -                * this loop.
> -                */
> -       }
> -
> -       return next_index;
> -}
> -
> -/**
>  * omap3_enter_idle_bm - Checks for any bus activity
>  * @dev: cpuidle device
>  * @drv: cpuidle driver
> @@ -254,7 +176,6 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>                                struct cpuidle_driver *drv,
>                               int index)
>  {
> -       int new_state_idx;
>        u32 core_next_state, per_next_state = 0, per_saved_state = 0, 
> cam_state;
>        struct omap3_idle_statedata *cx;
>        int ret;
> @@ -265,7 +186,7 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>         */
>        cam_state = pwrdm_read_pwrst(cam_pd);
>        if (cam_state == PWRDM_POWER_ON) {
> -               new_state_idx = drv->safe_state_index;
> +               index = drv->safe_state_index;
>                goto select_state;
>        }
>

Re: [RFC][PATCH 3/9] ARM: OMAP3: cpuidle - set enable_off_mode as static

[Jean Pihet]

On Fri, Mar 23, 2012 at 10:26 AM, Daniel Lezcano
 wrote:
> This variable is only used in the pm-debug.c file.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-omap2/pm-debug.c |    2 +-
>  arch/arm/mach-omap2/pm.h       |   30 --
>  2 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 814bcd9..86aa398 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -38,7 +38,7 @@
>  #include "prm2xxx_3xxx.h"
>  #include "pm.h"
>
> -u32 enable_off_mode;
> +static u32 enable_off_mode;
It is stil needed in cpuidle34xx.c.

>
>  #ifdef CONFIG_DEBUG_FS
>  #include 
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index af73a86..f5c3072 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -25,6 +25,18 @@ extern int omap4_idle_init(void);
>  extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
>  extern int (*omap_pm_suspend)(void);
>
> +/*
> + * cpuidle mach specific parameters
> + *
> + * The board code can override the default C-states definition using
> + * omap3_pm_init_cpuidle
> + */
> +struct cpuidle_params {
> +       u32 exit_latency;       /* exit_latency = sleep + wake-up latencies */
> +       u32 target_residency;
> +       u8 valid;               /* validates the C-state */
> +};
> +
Is this move needed? This will be removed in a later patch.

>  #if defined(CONFIG_PM_OPP)
>  extern int omap3_opp_init(void);
>  extern int omap4_opp_init(void);
> @@ -39,27 +51,9 @@ static inline int omap4_opp_init(void)
>  }
>  #endif
>
> -/*
> - * cpuidle mach specific parameters
> - *
> - * The board code can override the default C-states definition using
> - * omap3_pm_init_cpuidle
> - */
> -struct cpuidle_params {
> -       u32 exit_latency;       /* exit_latency = sleep + wake-up latencies */
> -       u32 target_residency;
> -       u8 valid;               /* validates the C-state */
> -};
> -
>  extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
>  extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
>
> -#ifdef CONFIG_PM_DEBUG
> -extern u32 enable_off_mode;
> -#else
> -#define enable_off_mode 0
> -#endif
> -
>  #if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
>  extern void pm_dbg_update_time(struct powerdomain *pwrdm, int prev);
>  #else
> --
> 1.7.5.4
>

Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC][PATCH 4/9] ARM: OMAP3: define cpuidle statically

[Jean Pihet]

On Fri, Mar 23, 2012 at 10:26 AM, Daniel Lezcano
 wrote:
> Use the new cpuidle API and define in the driver the states.
>
> Signed-off-by: Daniel Lezcano 
> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   85 +---
>  1 files changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c 
> b/arch/arm/mach-omap2/cpuidle34xx.c
> index 65b4e7aa..62e3cfd 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -228,23 +228,67 @@ DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>  struct cpuidle_driver omap3_idle_driver = {
>        .name =         "omap3_idle",
>        .owner =        THIS_MODULE,
> +       .states = {
> +               {
> +                       .enter            = omap3_enter_idle,
> +                       .exit_latency     = 2 + 2,
> +                       .target_residency = 5,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C1",
> +                       .desc             = "MPU ON + CORE ON",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 10 + 10,
> +                       .target_residency = 30,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C2",
> +                       .desc             = "MPU ON + CORE ON",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 50 + 50,
> +                       .target_residency = 300,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C3",
> +                       .desc             = "MPU RET + CORE ON",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 1500 + 1800,
> +                       .target_residency = 4000,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C4",
> +                       .desc             = "MPU OFF + CORE ON",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 2500 + 7500,
> +                       .target_residency = 12000,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C5",
> +                       .desc             = "MPU RET + CORE RET",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 3000 + 8500,
> +                       .target_residency = 15000,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C6",
> +                       .desc             = "MPU OFF + CORE RET",
> +               },
> +               {
> +                       .enter            = omap3_enter_idle_bm,
> +                       .exit_latency     = 1 + 3,
> +                       .target_residency = 3,
> +                       .flags            = CPUIDLE_FLAG_TIME_VALID,
> +                       .name             = "C7",
> +                       .desc             = "MPU OFF + CORE OFF",
> +               },
C7 is enabled depending on the errata i583, cf. below.

> +       },
> +       .state_count = OMAP3_NUM_STATES,
>  };
>
> -/* Helper to fill the C-state common data*/
> -static inline void _fill_cstate(struct cpuidle_driver *drv,
> -                                       int idx, const char *descr)
> -{
> -       struct cpuidle_state *state = &drv->states[idx];
> -
> -       state->exit_latency     = cpuidle_params_table[idx].exit_latency;
> -       state->target_residency = cpuidle_params_table[idx].target_residency;
> -       state->flags            = CPUIDLE_FLAG_TIME_VALID;
> -       state->enter            = omap3_enter_idle_bm;
> -       sprintf(state->name, "C%d", idx + 1);
> -       strncpy(state->desc, descr, CPUIDLE_DESC_LEN);
> -
> -}
> -
>  /* Helper to register the driver_data */
>  static inline struct omap3_idle_statedata *_fill_cstate_usage(
>                                        struct cpuidle_device *dev,
> @@ -277,50 +321,40 @@ int __init omap3_idle_init(void)
>        cam_pd = pwrdm_lookup("cam_pwrdm");
>
>
> -       drv->safe_state_index = -1;
>        dev = &per_cpu(omap3_idle_dev, smp_processor_id());
>
>        /* C1 . MPU WFI + Core active */
> -       _fill_cstate(drv, 0, "MPU ON + CORE ON");
> -       (&drv->states[0])->enter = omap3_enter_idle;
> -       drv->safe_state_index = 0;
Same minor remark about the safe_state_index as in the previous series.

>        cx = _fill_

Re: [RFC][PATCH 4/9] ARM: OMAP3: define cpuidle statically

[Jean Pihet]

On Fri, Mar 23, 2012 at 1:41 PM, Daniel Lezcano
 wrote:
> On 03/23/2012 01:35 PM, Jean Pihet wrote:
>>
>> On Fri, Mar 23, 2012 at 10:26 AM, Daniel Lezcano
>>   wrote:
>>>
>>> Use the new cpuidle API and define in the driver the states.
>>>
>>> Signed-off-by: Daniel Lezcano
>>> ---
>
>
> [ ... ]
>
>
>>>        /*
>>>         * Erratum i583: implementation for ES rev<  Es1.2 on 3630. We
>>> cannot
>>
>> C7 is enabled only if the errata does not apply.
>> This causes a problem with the statically defined C-states and also
>> with the per C-state sysfs disable knob. A consistency check should be
>> performed before trying to enter a state.
>
>
> In the patch 5/9, I decrement the state_count if the errata applies. I
> believe it is still compatible with the static C-states.
Correct! That solves the issue with the statically defined C-states.
The (dynamic) per C-state sysfs disable knob requires a runtime check
on .disable though, in case the cpuidle chosen C-state is demoted
(depending on the value of enable_off_mode and later the latency
constraints on the power domain).

>
>
>>> @@ -338,7 +372,6 @@ int __init omap3_idle_init(void)
>>>        drv->state_count = OMAP3_NUM_STATES;
>>>        cpuidle_register_driver(&omap3_idle_driver);
>>>
>>> -       dev->state_count = OMAP3_NUM_STATES;
>>>        if (cpuidle_register_device(dev)) {
>>>                printk(KERN_ERR "%s: CPUidle register device failed\n",
>>>                       __func__);
>>> --
>>> 1.7.5.4
>>>
>>
Thanks!
Jean

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

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH 13/17] ARM: OMAP3: cpuidle - use omap3_idle_data directly

[Jean Pihet]

Daniel,

On Wed, Apr 4, 2012 at 11:42 AM, Daniel Lezcano
 wrote:
> We are storing the 'omap3_idle_data' in the private data field
> if the cpuidle device. As we are using this variable only in this file,
Typo: _of_ the cpuidle device.

> that does not really make sense. Let's use the global variable directly
> instead dereferencing pointers in an idle critical loop.
>
> As the table is initialized statically, let's remove the initialization at
> startup too.
>
> Also, that simplfies the code.
>
> Signed-off-by: Daniel Lezcano 

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH 00/17][OMAP2/3] cpuidle34xx and cpuidle44xx cleanups

[Jean Pihet]

Daniel,

On Wed, Apr 4, 2012 at 11:42 AM, Daniel Lezcano
 wrote:
> This patchset makes some cleanup on these cpuidle drivers
> and consolidate the code across both architecture.
>
> Tested on OMAP3 (igepV2).
> Partially tested on OMAP4 (pandaboard), without offlining the cpu1.

Ok with this patch set, except the very minor remarks on 2 patches.

FWIW:
Reviewed-by: Jean Pihet 

Thanks,
Jean

>
> Daniel Lezcano (17):
>  ARM: OMAP4: cpuidle - Remove unused valid field
>  ARM: OMAP4: cpuidle - Declare the states with the driver declaration
>  ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table
>  ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
>  ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time
>  ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly
>  ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot
>    time
>  ARM: OMAP3: cpuidle - remove rx51 cpuidle parameters table
>  ARM: OMAP3: define cpuidle statically
>  ARM: OMAP3: cpuidle - remove the 'valid' field
>  ARM: OMAP3: cpuidle - remove cpuidle_params_table
>  ARM: OMAP3: define statically the omap3_idle_data
>  ARM: OMAP3: cpuidle - use omap3_idle_data directly
>  ARM: OMAP3 : cpuidle - simplify next_valid_state
>  ARM: OMAP3: set omap3_idle_data as static
>  ARM: OMAP3/4: consolidate cpuidle Makefile
>  ARM: OMAP3: cpuidle - set global variables static
>
>  arch/arm/mach-omap2/Makefile      |   11 +-
>  arch/arm/mach-omap2/board-rx51.c  |   38 +++---
>  arch/arm/mach-omap2/cpuidle34xx.c |  299 
> +++--
>  arch/arm/mach-omap2/cpuidle44xx.c |  135 +++--
>  arch/arm/mach-omap2/pm.h          |   38 ++---
>  5 files changed, 219 insertions(+), 302 deletions(-)
>
> --
> 1.7.5.4
>
>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH 06/17] ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly

[Jean Pihet]

Hi Daniel,

On Wed, Apr 4, 2012 at 11:42 AM, Daniel Lezcano
 wrote:
> We are storing the 'omap4_idle_data' in the private data field
> if the cpuidle device. As we are using this variable only in this file,
Typo: _of_ the cpuidle device.

> that does not really make sense. Let's use the global variable directly
> instead dereferencing pointers in an idle critical loop.
>
> Also, that simplfies the code.
>
> Signed-off-by: Daniel Lezcano 
> Reviewed-by: Jean Pihet 
> Reviewed-by: Santosh Shilimkar 

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC 1/4] ARM: topology: Add arch_scale_freq_power function

[Jean Pihet]

Hi Vincent,

On Tue, Jun 12, 2012 at 2:02 PM, Vincent Guittot
 wrote:
> Add infrastructure to be able to modify the cpu_power of each core
>
> Signed-off-by: Vincent Guittot 
> ---
>  arch/arm/include/asm/topology.h |    2 ++
>  arch/arm/kernel/topology.c      |   36 +++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 58b8b84..78e4c85 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -27,11 +27,13 @@ void init_cpu_topology(void);
>  void store_cpu_topology(unsigned int cpuid);
>  const struct cpumask *cpu_coregroup_mask(int cpu);
>
> +void set_power_scale(unsigned int cpu, unsigned long power);
>  #else
>
>  static inline void init_cpu_topology(void) { }
>  static inline void store_cpu_topology(unsigned int cpuid) { }
>
> +static inline void set_power_scale(unsigned int cpu, unsigned long power) { }
>  #endif
>
>  #include 
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 8200dea..00301a7 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -22,6 +22,35 @@
>  #include 
>  #include 
>
> +/*
> + * cpu power scale management
> + */
> +
> +/*
> + * cpu power table
> + * This per cpu data structure describes the relative capacity of each core.
> + * On a heteregenous system, cores don't have the same computation capacity
> + * and we reflect that difference in the cpu_power field so the scheduler can
> + * take this difference into account for load balance. A per cpu structure is
> + * preferred because each cpu is mainly using its own cpu_power even it's not
> + * always true because of nohz_idle_balance
The end of the comment is unclear IMO; Can you give more details on
the relation between cpu_power and nohz_idle_balance?

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [RFC 3/4] ARM: topology: Update cpu_power according to DT information

[Jean Pihet]

Vincent,

On Tue, Jun 12, 2012 at 2:02 PM, Vincent Guittot
 wrote:
> Use cpu compatibility field and clock-frequency field of DT to
> estimate the capacity of each core of the system
>
> Signed-off-by: Vincent Guittot 
> ---
>  arch/arm/kernel/topology.c |  122 
> 
>  1 file changed, 122 insertions(+)
>
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 2f85a64..0c2aee4 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -47,6 +48,122 @@ void set_power_scale(unsigned int cpu, unsigned long 
> power)
>        per_cpu(cpu_scale, cpu) = power;
>  }
>
> +#ifdef CONFIG_OF
> +struct cpu_efficiency {
> +       const char *compatible;
> +       unsigned long efficiency;
> +};
> +
> +/*
> + * Table of relative efficiency of each processors
> + * The efficiency value must fit in 20bit. The final
> + * cpu_scale value must be in the range [1:2048[.
Typo here.

> + * Processors that are not defined in the table,
> + * use the default SCHED_POWER_SCALE value for cpu_scale.
> + */
> +struct cpu_efficiency table_efficiency[] = {
> +       {"arm,cortex-a15", 3891},
> +       {"arm,cortex-a7",  2048},
How are those results measured or computed? Is this purely related to
the number crunching performance?

Also more generally what if the cores frequencies are changing?

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [linux-pm] [RFC 1/4] cpuidle: define the enter function in the driver structure

[Jean Pihet]

Hi Daniel,

On Fri, Jun 8, 2012 at 11:34 PM, Daniel Lezcano
 wrote:
> On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
>> Hi Daniel,
>
> Hi Deepthi,
>
>> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
>>
>>> We have the state index passed as parameter to the 'enter' function.
>>> Most of the drivers assign their 'enter' functions several times in
>>> the cpuidle_state structure, as we have the index, we can delegate
>>> to the driver to handle their own callback array.
>>>
>>> That will have the benefit of removing multiple lines of code in the
>>> different drivers.
>>>
>>> In order to smoothly modify the driver, the 'enter' function are in
>>> the driver structure and in the cpuidle state structure. That will
>>> let the time to modify the different drivers one by one.
>>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>>> assigned in the driver structure and use it, otherwise it invokes
>>> the 'enter' assigned to the cpuidle_state.
>>
>>
>> Currently, the backend driver initializes
>> all the cpuidle states supported on the platform,
>> and each state can have its own enter routine
>> which can be unique This is a clean approach.
>
> Yes, I perfectly understood the purpose of this field but as clean it is
> it does not make sense as it is not used in this way. If it is supposed
> to be done in the way you are describing here, we should have the same
> number of states and enter functions. Here it is how it is used:
>
>  --
> | Arch             | nr states | nr enter function |
>  --
> | x86 (nehalem)    |    3      |         1         |
>  --
> | x86 (snb)        |    4      |         1         |
>  --
> | x86 (atom)       |    4      |         1         |
>  --
> | ARM tegra        |    1      |         1         |
>  --
> | ARM omap3        |    7      |         2         |
>  --
> | ARM omap4        |    3      |         1         |
>  --
> | ARM ux500        |    2      |         1         |
>  --
> | ARM shmobile     |    1      |         1         |
>  --
> | ARM davinci      |    2      |         1         |
>  --
> | ARM at91         |    2      |         1         |
>  --
> | ARM s3c64xx      |    1      |         1         |
>  --
> | ARM exynos       |    2      |         1         |
>  --
> | ARM kirkwood     |    2      |         1         |
>  --
> | SH               |    3      |         1         |
>  --
> | PPC              |    2      |         2         |
>  --
> |                  |           |                   |
> | TOTAL            |    39     |        17         |
> |                  |           |                   |
>  --
>
>
> As you can see most of the enter functions are only used as one.
> The Omap3 cpuidle driver enter function for C2 calls the enter function
> of C1. Other arch, already use a table of callbacks or the index.
There is a plan to remove the extra enter function as part of an
optimization, cf. [1]. The fix is planned to reach the 3.6 mainline
kernel via Kevin's tree [2].

[1] http://marc.info/?l=linux-omap&m=133856365818099&w=2
[2] 
http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/for_3.6/pm/performance

The result is that there will be only one enter function for OMAP3.

Regards,
Jean

>> By moving the enter routine into the driver,
>> we are enforcing in having only one enter state.
>> There is unnecessary overhead involved
>> in calling a wrapper routine just to
>> index into the right idle state routine
>> for many platforms at runtime.
>
> I don't agree. For the sake of encapsulated code, we duplicate n-times a
> field and that is not used in this way. It is quite easy to have in the
> driver specific code a common enter function to ventilate to the right
> routine without adding extra overhead and let the common code use a
> single enter routine (which is already the case today).
>
>
> ___
> linux-pm mailing list
> linux...@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

___
linaro-dev mailing list
linaro-dev@lists.lina

Re: [RFC 3/4] ARM: topology: Update cpu_power according to DT information

[Jean Pihet]

Hi Amit, Peter,

On Wed, Jun 13, 2012 at 2:47 PM, Peter Zijlstra  wrote:
> On Wed, 2012-06-13 at 18:14 +0530, Amit Kucheria wrote:
>> Various discussions around power-aware scheduling have amplified the
>> need for the scheduler to have some knowledge of DVFS. This would then
>> require the scheduler to track 'cpu_power' ( = max power) and perhaps
>> a new variable 'current_power' that is changed by the DVFS framework.
>
> Note that capacity is in fact a better term -- not to be confused with
> the capacity as currently in use within the load-balancer. Luckily
> there's no way to read that an not be confused.. uhmm :-)
>>
>> The first goal, though, is to make sure that the scheduler can handle
>> different cpu_power values due to asymmetric cores.
>
> I would think the very first goal was to do a simple packing balancer
> before doing silly things with asymmetric setups.. but what do I know.
Sure! First things first ;p

Thanks for the details!

Regards,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: cpuidle future and improvements

[Jean Pihet]

Hi Daniel,

On Mon, Jun 18, 2012 at 2:55 PM, Daniel Lezcano
 wrote:
> On 06/18/2012 02:53 PM, Peter De Schrijver wrote:
>> On Mon, Jun 18, 2012 at 02:35:42PM +0200, Daniel Lezcano wrote:
>>> On 06/18/2012 01:54 PM, Deepthi Dharwar wrote:
 On 06/18/2012 02:10 PM, Daniel Lezcano wrote:

>
> Dear all,
>
> A few weeks ago, Peter De Schrijver proposed a patch [1] to allow per
> cpu latencies. We had a discussion about this patchset because it
> reverse the modifications Deepthi did some months ago [2] and we may
> want to provide a different implementation.
>
> The Linaro Connect [3] event bring us the opportunity to meet people
> involved in the power management and the cpuidle area for different SoC.
>
> With the Tegra3 and big.LITTLE architecture, making per cpu latencies
> for cpuidle is vital.
>
> Also, the SoC vendors would like to have the ability to tune their cpu
> latencies through the device tree.
>
> We agreed in the following steps:
>
> 1. factor out / cleanup the cpuidle code as much as possible
> 2. better sharing of code amongst SoC idle drivers by moving common bits
> to core code
> 3. make the cpuidle_state structure contain only data
> 4. add a API to register latencies per cpu
That makes sense, especially if you can refactor _and_ add new
functionality at the same time.

 On huge systems especially  servers, doing a cpuidle registration on a
 per-cpu basis creates a big overhead.
 So global registration was introduced in the first place.

 Why not have it as a configurable option or so ?
 Architectures having uniform cpuidle state parameters can continue to
 use global registration, else have an api to register latencies per cpu
 as proposed. We can definitely work to see the best way to implement it.
>>>
>>> Absolutely, this is one reason I think adding a function:
>>>
>>> cpuidle_register_latencies(int cpu, struct cpuidle_latencies);
>>>
>>> makes sense if it is used only for cpus with different latencies.
>>> The other architecture will be kept untouched.
Do you mean by keeping the parameters in the cpuidle_driver struct and
not calling the new API? That looks great.

>>>
>>> IMHO, before adding more functionalities to cpuidle, we should cleanup
>>> and consolidate the code. For example, there is a dependency between
>>> acpi_idle and intel_idle which can be resolved with the notifiers, or
>>> there is intel specific code in cpuidle.c and cpuidle.h, cpu_relax is
>>> also introduced to cpuidle which is related to x86 not the cpuidle core,
>>> etc ...
>>>
>>> Cleanup the code will help to move the different bits from the arch
>>> specific code to the core code and reduce the impact of the core's
>>> modifications. That should let a common pattern to emerge and will
>>> facilitate the modifications in the future (per cpu latencies is one of
>>> them).
>>>
>>> That will be a lot of changes and this is why I proposed to put in place
>>> a cpuidle-next tree in order to consolidate all the cpuidle
>>> modifications people is willing to see upstream and provide better testing.
Nice! The new tree needs to be as close as possible to mainline
though. Do you have plans for that?
Do not hesitate to ask for help on OMAPs!

Regards,
Jean

>>
>> Sounds like a good idea. Do you have something like that already?
>
> Yes but I need to cleanup the tree before.
>
> http://git.linaro.org/gitweb?p=people/dlezcano/linux-next.git;a=summary
>
> --
>   Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [URGENT] Perf report shared objects showing unknown on ARM

[Jean Pihet]

Hello,


On 15 May 2014 07:36, sneha priya  wrote:
> Hello,
>
> There is an issue related to perf which I am facing since 15 days. Hoping
> that the great minds here will help me to solve this.
>
> I have a requirement to make perf tool work on a device having ARM
> architecture. But, on recording the tracepoint events and then running
> ./perf report, it shows the shared objects name as [unknown] and Symbols as
> 0, whereas for software and hardware events I do not experience this
> issue.
> I have cross compiled the perf tool available on mailine and ported it on a
> device having ARM architecture.
>
> Output on ARM based device with kernel 3.4.
>
> ./perf record -e kmem:kmalloc cal
>
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.007 MB perf.data (~321 samples) ]
>
> ./perf report
>
> OverheadCommand Shared Object
> Symbol
> ...   
> ...
> 40.78%   cal   [unknown]
> [.]
> 31.6%   cal   [unknown]
> [.]
>
> [...]
>
>  On ubuntu 12.04, system  (kernel 3.10) it works perfectly fine.
>
> Output on x86 architecture
>
> ./perf record -e kmem:kmalloc gcalctool
>
> [ perf record: Woken up 3 times to write data ]
> [ perf record: Captured and wrote 0.27 MB perf.data (~845 samples) ]
>
> ./perf report --stdio
>
> Overhead  Command Shared
> Object Symbol
> ...   
> ...
> 96.55%  cal
> [kernel.kallsyms]   [k] kmem_cache_alloc_trace
> 3.45%cal
> [kernel.kallsyms]  [.]__kmalloc
>
> [...]
>
>
> Keenly, awaiting for you help.

There are a few things to check. Note that the 3.4 kernel is OK wrt
perf and tracepoints but is quite old, the recent development now
happens on 3.15.

- Do you have kallsyms enabled in your kernel (CONFIG_KALLSYMS=y)?
- Are there warnings issued by perf record, e.g. access to kernel symbols etc?
- perf report needs to know about your vmlinux image (the one that
contains the debug symbols etc in the root directory of the kernel
build). You can use '-k ' or '--vmlinux=', cf. perf report
--help.
- You can dump the samples from perf.data using perf report -D. The
entries with 'PERF_RECORD_SAMPLE(IP, 2)' are for the user space apps.
- The '-v' option gives more info. This can be used multiple times ('-vv').

>
> Thanks.
>
> Sneha.

Regards,
Jean

>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [URGENT] Perf report shared objects showing unknown on ARM

[Jean Pihet]

Hello,

Indeed there is a problem in the ARM code for tracepoints.
After a good discussion with the perf maintainers a solution has be
found, cf. http://www.spinics.net/lists/arm-kernel/msg332293.html.

Can you check if this fixes the problem? It does on my side on 3.15-rc4.

The patch is under review by the ARM experts and hopefully should be
merged soon.

Regards,
Jean


On 16 May 2014 09:34, Jean Pihet  wrote:
> Hello,
>
>
> On 15 May 2014 07:36, sneha priya  wrote:
>> Hello,
>>
>> There is an issue related to perf which I am facing since 15 days. Hoping
>> that the great minds here will help me to solve this.
>>
>> I have a requirement to make perf tool work on a device having ARM
>> architecture. But, on recording the tracepoint events and then running
>> ./perf report, it shows the shared objects name as [unknown] and Symbols as
>> 0, whereas for software and hardware events I do not experience this
>> issue.
>> I have cross compiled the perf tool available on mailine and ported it on a
>> device having ARM architecture.
>>
>> Output on ARM based device with kernel 3.4.
>>
>> ./perf record -e kmem:kmalloc cal
>>
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.007 MB perf.data (~321 samples) ]
>>
>> ./perf report
>>
>> OverheadCommand Shared Object
>> Symbol
>> ...   
>> ...
>> 40.78%   cal   [unknown]
>> [.]
>> 31.6%   cal   [unknown]
>> [.]
>>
>> [...]
>>
>>  On ubuntu 12.04, system  (kernel 3.10) it works perfectly fine.
>>
>> Output on x86 architecture
>>
>> ./perf record -e kmem:kmalloc gcalctool
>>
>> [ perf record: Woken up 3 times to write data ]
>> [ perf record: Captured and wrote 0.27 MB perf.data (~845 samples) ]
>>
>> ./perf report --stdio
>>
>> Overhead  Command Shared
>> Object Symbol
>> ...   
>> ...
>> 96.55%  cal
>> [kernel.kallsyms]   [k] kmem_cache_alloc_trace
>> 3.45%cal
>> [kernel.kallsyms]  [.]__kmalloc
>>
>> [...]
>>
>>
>> Keenly, awaiting for you help.
>
> There are a few things to check. Note that the 3.4 kernel is OK wrt
> perf and tracepoints but is quite old, the recent development now
> happens on 3.15.
>
> - Do you have kallsyms enabled in your kernel (CONFIG_KALLSYMS=y)?
> - Are there warnings issued by perf record, e.g. access to kernel symbols etc?
> - perf report needs to know about your vmlinux image (the one that
> contains the debug symbols etc in the root directory of the kernel
> build). You can use '-k ' or '--vmlinux=', cf. perf report
> --help.
> - You can dump the samples from perf.data using perf report -D. The
> entries with 'PERF_RECORD_SAMPLE(IP, 2)' are for the user space apps.
> - The '-v' option gives more info. This can be used multiple times ('-vv').
>
>>
>> Thanks.
>>
>> Sneha.
>
> Regards,
> Jean
>
>>
>> ___
>> linaro-dev mailing list
>> linaro-dev@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-dev
>>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [URGENT] Perf report shared objects showing unknown on ARM

[Jean Pihet]

Hi

On 19 May 2014 16:08, Prankul Garg  wrote:
> Jean Pihet  writes:
>
>>
>> Hello,
>>
>> Indeed there is a problem in the ARM code for tracepoints.
>> After a good discussion with the perf maintainers a solution has be
>> found, cf. http://www.spinics.net/lists/arm-kernel/msg332293.html.
>>
>> Can you check if this fixes the problem? It does on my side on 3.15-rc4.
>
> Hi Jean,
>
> yes, it solved the problem but partially, i am getting different outputs for
> x86 and ARM.
>
> On x86(3.10.28):
>
> ./perf record -e kmem:kmalloc cal
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.009 MB perf.data (~381 samples) ]
>
> ./perf report
> # Overhead  Command  Shared Object  Symbol
> #   ...  .  ..
> #
> 96.77%  cal  [kernel.kallsyms]  [k] kmem_cache_alloc_trace
>  3.23%  cal  [kernel.kallsyms]  [k] __kmalloc
>
> ./perf report -v
>96.77%  cal  /lib/modules/3.10.28+/build/vmlinux  0x81166f6d
> v [k] kmem_cache_alloc_trace
> 3.23%  cal  /lib/modules/3.10.28+/build/vmlinux  0x81166b24
> v [k] __kmalloc
>
> ./perf buildid-list
> aef9a24fcddff67cd67bcc1fd27dbeaf86d35487 [kernel.kallsyms]
>
>
> but on ARM(3.4.0):
>
> ./perf record -e kmem:kmalloc cal
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.004 MB perf.data (~192 samples) ]
>
> ./perf report
> # Overhead  Command  Shared Object  Symbol
> #   ...  .  ..
> #
>100.00%  cal [ks8851]   [k] 0x01247890
>
> ./perf report -v
>100.00%  cal /lib/modules/3.4.0/.../ks8851.ko  0x1247890 l [k]
> 0x01247890
>
> ./perf buildid-list
> d0898c01486575dddafc6686240186bdb4b4430a /lib/modules/3.4.0/.../ks8851.ko
>
>
> that means on ARM, it is not getting the buildid for [kernel.kallsyms].
>
> Are you getting the same output for ARM and x86 ??
Yes. Here is the output I have:

root@axp-linaro:/home/linaro/linaro/kernel# ./linux/tools/perf/perf report -v
build id event received for vmlinux: 6db809b8adab308f64037970799dc836f6b96422
Looking at the vmlinux_path (6 entries long)
Using /proc/kallsyms for symbols
# To display the perf.data header info, please use --header/--header-only option
#
# Samples: 12  of event 'kmem:kmalloc'
# Event count (approx.): 12
#
# Overhead   Command  Shared Object   Sy
#     .  ...
#
91.67%  stress_bt_v7  [kernel.kallsyms]  0xc00f1548 k [k] kmem_cache_alloc_t
 8.33%  stress_bt_v7  [kernel.kallsyms]  0xc00f1400 k [k] __kmalloc


#
# (For a higher level overview, try: perf report --sort comm,dso)
#

root@axp-linaro:/home/linaro/linaro/kernel/linux# ./tools/perf/perf
buildid-list
6db809b8adab308f64037970799dc836f6b96422 vmlinux
root@axp-linaro:/home/linaro/linaro/kernel/linux#

perf is looking for symbols in vmlinux (if present in the current dir
or specified in the command line) and if not found from kallsyms. So
if I run 'perf report' from the kernel root dir, the symbols are from
vmlinux; if I run it from elsewhere the symbols are from kallsyms.

Also your build is quite old, a lot of changes went into perf since 3.4.

Regards,
Jean

>
> regards,
> Prankul Garg
>
>>
>> The patch is under review by the ARM experts and hopefully should be
>> merged soon.
>>
>> Regards,
>> Jean
>>
>> On 16 May 2014 09:34, Jean Pihet  wrote:
>> > Hello,
>> >
>> >
>> > On 15 May 2014 07:36, sneha priya  wrote:
>> >> Hello,
>> >>
>> >> There is an issue related to perf which I am facing since 15 days.
> Hoping
>> >> that the great minds here will help me to solve this.
>> >>
>> >> I have a requirement to make perf tool work on a device having ARM
>> >> architecture. But, on recording the tracepoint events and then running
>> >> ./perf report, it shows the shared objects name as [unknown] and
> Symbols as
>> >> 0, whereas for software and hardware events I do not experience
> this
>> >> issue.
>> >> I have cross compiled the perf tool available on mailine and ported it
> on a
>> >> device having ARM architecture.
>> >>
>> >> Output on ARM based device with kernel 3.4.
>> >>
>> >> ./perf record -e kmem:kmalloc cal
>> >>
>> >> [ perf record: Woken up 1 times to write data ]
>> >> [ perf record: Captured a

Re: [URGENT] Perf report shared objects showing unknown on ARM

[Jean Pihet]

Hi,

On 21 May 2014 16:04, Prankul Garg  wrote:
> Jean Pihet  writes:
>
>
>
>> Also your build is quite old, a lot of changes went into perf since 3.4.
>
> Hi Jean,
>
> finally, it works for me too. The problem i was facing due to the older
> kernel.
That is great!

The proposed patch is under review and should be mainlined soon.

Regards,
Jean

>
> Thanks & regards,
> Prankul Garg
>
>>
>> Regards,
>> Jean
>
>
>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Foundation model stays in idle

[Jean Pihet]

Hi!

I wanted to ask for help on the Foundation model which becomes really
slow after a while, to the point where it is not usable anymore.
Quite often I have to kill the model (Ctrl-C) and restart it, with
corrupted files on the root FS.

I am using the latest Foundation model, kernel image, instructions
from the release page [1] but it also happens with my own mainline
kernel.

[1] http://releases.linaro.org/latest/openembedded/aarch64/

Symptoms and observations:
- after a few minutes the system looks like it is hanging but still
reacts on ping, the shell is still active but everything related to
the filesystem hangs (top, free, compiler etc.),
- there is no sign of memory exhaustion or abnormal caching as seen
from a recurrent 'free'. 90% of the memory is free,
- the problem arrives faster when working in an NFS mounted dir,
- on the visualization webpage (localhost:2001) all cores are idle all
the time and the number of instructions executed is really low (about
30K insts/s),
- the exact same setup used to work fine until my machine crashed
(lost the home dir completely), now the problems happens with both AXF
and UEFI based boot.

Is there something you already observed, is there a cure (kernel or
root FS config etc.)?

Here is the startup script:
# cat boot_fvp_uefi.sh
#!/bin/sh

# Start Foundation model >= 5206 from the Image and img images
pidof Foundation_v8 > /dev/null && echo -n 'Foundation_v8 already
running' && exit

IMAGE=vexpress64-openembedded_lamp-armv8-gcc-4.8_20140417-630.img

exec Foundation_v8\
--cores=2\
 --network bridged\
 --block-device $IMAGE\
 --no-secure-memory\
 --visualization\
 --gicv3\
 --data=bl1.bin@0x0\
 --data=uefi_fvp-base.bin@0x800

Any feedback welcome!

Cheers,
Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: Common ARM context save/restore code

[Jean Pihet]

Hi Jon,

That is a nice follow up to the OMAP idle code clean up, cf.
http://lists.linaro.org/pipermail/linaro-dev/2010-October/001084.html.
It would be nice to see how the code is organized and the impact of
integrating it into the kernel.

Regards,
Jean

On Tue, Oct 12, 2010 at 12:39 PM, Jon Callan  wrote:
> Vishwa,
>
> I have a more-or-less complete set of example code for CPU context 
> save/restore, currently supporting A5/A8/A9 and with planned support for 
> Eagle.
>
> It is structured as "firmware" at the moment, but it would be much better if 
> it was integrated into the ARM Linux kernel. The idea is the kernel calls it 
> from CPUidle, and it saves all CPU context and cuts the power. Then when 
> power returns, it restores all CPU context and returns to the kernel as if 
> nothing has happened.
>
> It handles just the CPU and cluster context, which on A9mpcore includes MMU, 
> GIC, VFP, SCU, L2cc, Debug, etc. It takes care of cleaning caches and 
> entering/leaving the coherency domain. There is also support for TrustZone, 
> but as you say that's quite platform-specific.
>
> So we would need to integrate this with the SoC-specific code somehow.
>
> Jon.
> --
> I work Tue/Wed/Fri only
> Jon Callan, Staff Software Engineer, Processor Division
> ARM Cambridge / +44 1223 400814
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose the 
> contents to any other person, use it for any purpose, or store or copy the 
> information in any medium.  Thank you.
>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to work withCONFIG_THUMB2_KERNEL

[Jean Pihet]

Hi Dave,

On Tue, Dec 7, 2010 at 11:16 AM, Dave Martin  wrote:
> On Tue, Dec 7, 2010 at 6:31 AM, Santosh Shilimkar
>  wrote:
>> Dave,
>>> -Original Message-
>>> From: linaro-dev-boun...@lists.linaro.org [mailto:linaro-dev-
>>> boun...@lists.linaro.org] On Behalf Of Dave Martin
>>> Sent: Monday, December 06, 2010 11:06 PM
>>> To: linux-arm-ker...@lists.infradead.org
>>> Cc: Tony Lindgren; Dave Martin; linux-o...@vger.kernel.org; linaro-
>>> d...@lists.linaro.org
>>> Subject: [PATCH v2 1/3] ARM: omap: Enable low-level omap3 PM code to
>> work
>>> withCONFIG_THUMB2_KERNEL
>>>
>
> [...]
>
>>
>> No need to mention but this patch changes lot of things around
>> power management code. You said " Tested on: omap3 (Beagle xM A2)"
>>
>> What did you test ? Is it just boot test ? For sure just boot
>> test is not enough for this patch and needs to test the PM.
>
> That's a fair point--- yes, I've only tested boot / reboot / shutdown so far.
>
The ASM idle code is being reworked right now, which means the T2
support will need to be reworked on top of the patches. Cf. [1] and
[2].

> If you have any thoughts about how to exercise the power management
> functionality more completely, I'd be happy to have a go...
Cf. [3] for more details on how to exercise the PM on the target. This
wiki page is slightly outdated but the idea is still the same. Note
that only cpuidle is currently supported correctly on l-o master.

[1] http://marc.info/?l=linux-omap&m=129139584919242&w=2
[2] http://marc.info/?l=linux-omap&m=129172034809447&w=2
[3] http://elinux.org/OMAP_Power_Management

>
> Cheers
> ---Dave

Thanks,
Jean

>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v5 5/5] ARM: omap3: Thumb-2 compatibility for sleep34xx.S

[Jean Pihet]

On Thu, Feb 17, 2011 at 1:42 PM, Dave Martin  wrote:
>  * Build unconditionally as ARM for correct interoperation with
>   OMAP firmware.
>
>  * Fix an out-of-range ADR when building for ARM.
>
>  * Remove deprecated PC-relative stores.
>
>  * Add the required ENDPROC() directive for each ENTRY().
>
>  * .align before data words.
>
>  * Handle non-interworking return from v7_flush_dcache_all.
>
> Signed-off-by: Dave Martin 

Reviewed OK
Reviewed-by: Jean Pihet 

Tested OK on OMAP3 with and without CONFIG_THUMB2_KERNEL set. PM
RETention and OFF modes in cpuidle OK.
Tested-by: Jean Pihet 

> ---
>  arch/arm/mach-omap2/sleep34xx.S |   48 ++
>  1 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index a05c348..f377724 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -64,6 +64,11 @@
>  #define SDRC_DLLA_STATUS_V     OMAP34XX_SDRC_REGADDR(SDRC_DLLA_STATUS)
>  #define SDRC_DLLA_CTRL_V       OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
>
> +/*
> + * This file needs be built unconditionally as ARM to interoperate correctly
> + * with non-Thumb-2-capable firmware.
> + */
> +       .arm
>
>  /*
>  * API functions
> @@ -82,6 +87,8 @@ ENTRY(get_restore_pointer)
>        stmfd   sp!, {lr}       @ save registers on stack
>        adr     r0, restore
>        ldmfd   sp!, {pc}       @ restore regs and return
> +ENDPROC(get_restore_pointer)
> +       .align
>  ENTRY(get_restore_pointer_sz)
>        .word   . - get_restore_pointer
>
> @@ -91,6 +98,8 @@ ENTRY(get_omap3630_restore_pointer)
>        stmfd   sp!, {lr}       @ save registers on stack
>        adr     r0, restore_3630
>        ldmfd   sp!, {pc}       @ restore regs and return
> +ENDPROC(get_omap3630_restore_pointer)
> +       .align
>  ENTRY(get_omap3630_restore_pointer_sz)
>        .word   . - get_omap3630_restore_pointer
>
> @@ -100,6 +109,8 @@ ENTRY(get_es3_restore_pointer)
>        stmfd   sp!, {lr}       @ save registers on stack
>        adr     r0, restore_es3
>        ldmfd   sp!, {pc}       @ restore regs and return
> +ENDPROC(get_es3_restore_pointer)
> +       .align
>  ENTRY(get_es3_restore_pointer_sz)
>        .word   . - get_es3_restore_pointer
>
> @@ -113,8 +124,10 @@ ENTRY(enable_omap3630_toggle_l2_on_restore)
>        stmfd   sp!, {lr}       @ save registers on stack
>        /* Setup so that we will disable and enable l2 */
>        mov     r1, #0x1
> -       str     r1, l2dis_3630
> +       adrl    r2, l2dis_3630  @ may be too distant for plain adr
> +       str     r1, [r2]
>        ldmfd   sp!, {pc}       @ restore regs and return
> +ENDPROC(enable_omap3630_toggle_l2_on_restore)
>
>        .text
>  /* Function to call rom code to save secure ram context */
> @@ -139,12 +152,14 @@ ENTRY(save_secure_ram_context)
>        nop
>        nop
>        ldmfd   sp!, {r1-r12, pc}
> +       .align
>  sram_phy_addr_mask:
>        .word   SRAM_BASE_P
>  high_mask:
>        .word   0x
>  api_params:
>        .word   0x4, 0x0, 0x0, 0x1, 0x1
> +ENDPROC(save_secure_ram_context)
>  ENTRY(save_secure_ram_context_sz)
>        .word   . - save_secure_ram_context
>
> @@ -279,8 +294,18 @@ clean_l2:
>         *  - 'might' have to copy address, load and jump to it
>         */
>        ldr     r1, kernel_flush
> -       mov     lr, pc
> -       bx      r1
> +       blx     r1
> +       /*
> +        * The kernel doesn't interwork: v7_flush_dcache_all in particluar 
> will
> +        * always return in Thumb state when CONFIG_THUMB2_KERNEL is enabled.
> +        * This sequence switches back to ARM.  Note that .align may insert a
> +        * nop: bx pc needs to be word-aligned in order to work.
> +        */
> + THUMB(        .thumb          )
> + THUMB(        .align          )
> + THUMB(        bx      pc      )
> + THUMB(        nop             )
> +       .arm
>
>  omap3_do_wfi:
>        ldr     r4, sdrc_power          @ read the SDRC_POWER register
> @@ -438,6 +463,7 @@ skipl2dis:
>  #endif
>        b       logic_l1_restore
>
> +       .align
>  l2_inv_api_params:
>        .word   0x1, 0x00
>  l2_inv_gp:
> @@ -607,6 +633,7 @@ usettbr0:
>
>  /* This function implements the erratum ID i443 WA, applies to 34xx >= ES3.0 
> */
>        .text
> +       .align  3
>  ENTRY(es3_sdrc_fix)
>        ldr     r4, sdrc_syscfg         @ get config addr
>        ldr     r5, [r4]                @ get value
> @@ -634,6 +661,7 @@ ENTRY(es3_sdrc_fix)
>        str     r5, [r4]                @ kick off refreshes
>        bx 

Re: [PATCH v5 3/5] ARM: omap3: Remove hand-encoded SMC instructions

[Jean Pihet]

On Thu, Feb 17, 2011 at 1:42 PM, Dave Martin  wrote:
> For various reasons, Linux now only officially supports being built
> with tools which are new enough to understand the SMC instruction.
>
> Replacing the hand-encoded instructions when the mnemonic also
> allows for correct assembly in Thumb-2 (otherwise, the result is
> random data in the middle of the code).
>
> The Makefile already ensures that this file is built with a high
> enough gcc -march= flag (armv7-a).
>
> Signed-off-by: Dave Martin 
> Tested-by: Santosh Shilimkar 

Reviewed OK
Reviewed-by: Jean Pihet 

Tested OK on OMAP3 with and without CONFIG_THUMB2_KERNEL set. PM
RETention and OFF modes in cpuidle OK.
Tested-by: Jean Pihet 

> ---
>  arch/arm/mach-omap2/sleep34xx.S |   14 +++---
>  1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
> index 98d8232..a05c348 100644
> --- a/arch/arm/mach-omap2/sleep34xx.S
> +++ b/arch/arm/mach-omap2/sleep34xx.S
> @@ -133,7 +133,7 @@ ENTRY(save_secure_ram_context)
>        mov     r6, #0xff
>        mcr     p15, 0, r0, c7, c10, 4  @ data write barrier
>        mcr     p15, 0, r0, c7, c10, 5  @ data memory barrier
> -       .word   0xE1600071              @ call SMI monitor (smi #1)
> +       smc     #1                      @ call SMI monitor (smi #1)
>        nop
>        nop
>        nop
> @@ -408,7 +408,7 @@ skipl2dis:
>        adr     r3, l2_inv_api_params   @ r3 points to dummy parameters
>        mcr     p15, 0, r0, c7, c10, 4  @ data write barrier
>        mcr     p15, 0, r0, c7, c10, 5  @ data memory barrier
> -       .word   0xE1600071              @ call SMI monitor (smi #1)
> +       smc     #1                      @ call SMI monitor (smi #1)
>        /* Write to Aux control register to set some bits */
>        mov     r0, #42                 @ set service ID for PPA
>        mov     r12, r0                 @ copy secure Service ID in r12
> @@ -419,7 +419,7 @@ skipl2dis:
>        ldr     r3, [r4, #0xBC]         @ r3 points to parameters
>        mcr     p15, 0, r0, c7, c10, 4  @ data write barrier
>        mcr     p15, 0, r0, c7, c10, 5  @ data memory barrier
> -       .word   0xE1600071              @ call SMI monitor (smi #1)
> +       smc     #1                      @ call SMI monitor (smi #1)
>
>  #ifdef CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE
>        /* Restore L2 aux control register */
> @@ -434,7 +434,7 @@ skipl2dis:
>        adds    r3, r3, #8              @ r3 points to parameters
>        mcr     p15, 0, r0, c7, c10, 4  @ data write barrier
>        mcr     p15, 0, r0, c7, c10, 5  @ data memory barrier
> -       .word   0xE1600071              @ call SMI monitor (smi #1)
> +       smc     #1                      @ call SMI monitor (smi #1)
>  #endif
>        b       logic_l1_restore
>
> @@ -443,18 +443,18 @@ l2_inv_api_params:
>  l2_inv_gp:
>        /* Execute smi to invalidate L2 cache */
>        mov r12, #0x1                   @ set up to invalidate L2
> -       .word 0xE1600070                @ Call SMI monitor (smieq)
> +       smc     #0                      @ Call SMI monitor (smieq)
>        /* Write to Aux control register to set some bits */
>        ldr     r4, scratchpad_base
>        ldr     r3, [r4,#0xBC]
>        ldr     r0, [r3,#4]
>        mov     r12, #0x3
> -       .word   0xE1600070              @ Call SMI monitor (smieq)
> +       smc     #0                      @ Call SMI monitor (smieq)
>        ldr     r4, scratchpad_base
>        ldr     r3, [r4,#0xBC]
>        ldr     r0, [r3,#12]
>        mov     r12, #0x2
> -       .word   0xE1600070              @ Call SMI monitor (smieq)
> +       smc     #0                      @ Call SMI monitor (smieq)
>  logic_l1_restore:
>        ldr     r1, l2dis_3630
>        cmp     r1, #0x1                @ Test if L2 re-enable needed on 3630
> --
> 1.7.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH v5 4/5] ARM: omap3: Thumb-2 compatibility for sram34xx.S

[Jean Pihet]

On Thu, Feb 17, 2011 at 1:42 PM, Dave Martin  wrote:
>  * Build unconditionally as ARM for correct interoperation with
>   OMAP firmware.
>
>  * Remove deprecated PC-relative stores
>
>  * Add the required ENDPROC() directive for each ENTRY().
>
>  * .align before data words
>
> Signed-off-by: Dave Martin 
> ---
>  arch/arm/mach-omap2/sram34xx.S |   36 
>  1 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
> index 7f893a2..fd1531c 100644
> --- a/arch/arm/mach-omap2/sram34xx.S
> +++ b/arch/arm/mach-omap2/sram34xx.S
> @@ -34,6 +34,12 @@
>  #include "sdrc.h"
>  #include "cm2xxx_3xxx.h"
>
> +/*
> + * This file needs be built unconditionally as ARM to interoperate correctly
> + * with non-Thumb-2-capable firmware.
> + */
> +       .arm
> +
>        .text
>
>  /* r1 parameters */
> @@ -116,24 +122,36 @@ ENTRY(omap3_sram_configure_core_dpll)
>
>                                        @ pull the extra args off the stack
>                                        @  and store them in SRAM
> +
> +/*
> + * PC-relative stores are deprecated in ARMv7 and lead to undefined behaviour
> + * in Thumb-2: use a r7 as a base instead.
> + * Be careful not to clobber r7 when maintaing this file.
Sorry I forgot about this minor typo: 'maintaining'

Jean

> + */
> + THUMB(        adr     r7, omap3_sram_configure_core_dpll                    
>   )
> +       .macro strtext Rt:req, label:req
> + ARM(  str     \Rt, \label                                             )
> + THUMB(        str     \Rt, [r7, \label - omap3_sram_configure_core_dpll]    
>   )
> +       .endm
> +
>        ldr     r4, [sp, #52]
> -       str     r4, omap_sdrc_rfr_ctrl_0_val
> +       strtext r4, omap_sdrc_rfr_ctrl_0_val
>        ldr     r4, [sp, #56]
> -       str     r4, omap_sdrc_actim_ctrl_a_0_val
> +       strtext r4, omap_sdrc_actim_ctrl_a_0_val
>        ldr     r4, [sp, #60]
> -       str     r4, omap_sdrc_actim_ctrl_b_0_val
> +       strtext r4, omap_sdrc_actim_ctrl_b_0_val
>        ldr     r4, [sp, #64]
> -       str     r4, omap_sdrc_mr_0_val
> +       strtext r4, omap_sdrc_mr_0_val
>        ldr     r4, [sp, #68]
> -       str     r4, omap_sdrc_rfr_ctrl_1_val
> +       strtext r4, omap_sdrc_rfr_ctrl_1_val
>        cmp     r4, #0                  @ if SDRC_RFR_CTRL_1 is 0,
>        beq     skip_cs1_params         @  do not use cs1 params
>        ldr     r4, [sp, #72]
> -       str     r4, omap_sdrc_actim_ctrl_a_1_val
> +       strtext r4, omap_sdrc_actim_ctrl_a_1_val
>        ldr     r4, [sp, #76]
> -       str     r4, omap_sdrc_actim_ctrl_b_1_val
> +       strtext r4, omap_sdrc_actim_ctrl_b_1_val
>        ldr     r4, [sp, #80]
> -       str     r4, omap_sdrc_mr_1_val
> +       strtext r4, omap_sdrc_mr_1_val
>  skip_cs1_params:
>        mrc     p15, 0, r8, c1, c0, 0   @ read ctrl register
>        bic     r10, r8, #0x800         @ clear Z-bit, disable branch 
> prediction
> @@ -271,6 +289,7 @@ skip_cs1_prog:
>        ldr     r12, [r11]              @ posted-write barrier for SDRC
>        bx      lr
>
> +       .align
>  omap3_sdrc_power:
>        .word OMAP34XX_SDRC_REGADDR(SDRC_POWER)
>  omap3_cm_clksel1_pll:
> @@ -319,6 +338,7 @@ omap3_sdrc_dlla_ctrl:
>        .word OMAP34XX_SDRC_REGADDR(SDRC_DLLA_CTRL)
>  core_m2_mask_val:
>        .word 0x07FF
> +ENDPROC(omap3_sram_configure_core_dpll)
>
>  ENTRY(omap3_sram_configure_core_dpll_sz)
>        .word   . - omap3_sram_configure_core_dpll
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [pytimechart] new cpuidle tracepoint

[Jean Pihet]

Hi Pierre,

On Mon, Feb 28, 2011 at 4:09 PM, Pierre Tardy  wrote:
> On Mon, Feb 28, 2011 at 2:43 PM, Vincent Guittot
>  wrote:
>> On 27 February 2011 17:31, Pierre Tardy  wrote:
>>> On Fri, Feb 18, 2011 at 11:03 AM, Vincent Guittot
>>>  wrote:
 Hi,

 I have started to use the new cpuidle tracepoint and created a plugin
 for pytimechart.
 I'm not sure if it's the right way to add this cpuidle trace format
 update but it's the less intrusive one.
>>>
>>> Yes, it is okay. I actually plan to also put the whole cpuidle trace
>>> handling into such plugin.
>>> Would you care to send me one of your trace file, so that I can
>>> actually test it.
>>
>> Yes, I have attached the trace file which is Vishwa's one in fact.
> Thanks, I applied and pushed the patch.
>
> Please note that your trace has some suspicious tracepoints with same
> timestamps for end of cpuidle and start of next cpuidle.
That comes from a known problem about the timer on OMAP. The 32K timer
is feeding the trace timestamps and so the granularity is not high
enough (30us).

> -0     [000]   270.645935: cpu_idle: state=2 cpu_id=0
> -0     [000]   271.020935: cpu_idle: state=4294967295 cpu_id=0
> -0     [000]   271.020935: cpu_idle: state=2 cpu_id=0
> -0     [000]   271.036560: cpu_idle: state=4294967295 cpu_id=0
> -0     [000]   271.073395: cpu_idle: state=1 cpu_id=0
>
>
> Also, cpuidle states name in pytimechart are very selfishly hardcoded
> with intel's convention.
> Can you tell what is your convention, so that we can think of a best
> way to handle display of state's name for ftrace text output?
The power trace points API is described in Documentation/trace/events-power.txt.

Here is an excerpt of it:
"
The value of '-1' or '4294967295' for state means an exit from the
current state,
i.e. trace_cpu_idle(4, smp_processor_id()) means that the system
enters the idle state 4, while trace_cpu_idle(PWR_EVENT_EXIT,
smp_processor_id())
means that the system exits the previous idle state.

The event which has 'state=4294967295' in the trace is very important
to the user
space tools which are using it to detect the end of the current state, and so to
correctly draw the states diagrams and to calculate accurate statistics etc.
"

I would be glad to help on pytimechart, please let me know!

Regards,
Jean

>
> Regards,
> Pierre
>
> ___
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [pytimechart] new cpuidle tracepoint

[Jean Pihet]

On Mon, Feb 28, 2011 at 4:49 PM, Pierre Tardy  wrote:
> On Mon, Feb 28, 2011 at 4:30 PM, Jean Pihet  wrote:
>> Hi Pierre,
>>
>> On Mon, Feb 28, 2011 at 4:09 PM, Pierre Tardy  wrote:
>>> On Mon, Feb 28, 2011 at 2:43 PM, Vincent Guittot
>>>  wrote:
>>>> On 27 February 2011 17:31, Pierre Tardy  wrote:
>>>>> On Fri, Feb 18, 2011 at 11:03 AM, Vincent Guittot
>>>>>  wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I have started to use the new cpuidle tracepoint and created a plugin
>>>>>> for pytimechart.
>>>>>> I'm not sure if it's the right way to add this cpuidle trace format
>>>>>> update but it's the less intrusive one.
>>>>>
>>>>> Yes, it is okay. I actually plan to also put the whole cpuidle trace
>>>>> handling into such plugin.
>>>>> Would you care to send me one of your trace file, so that I can
>>>>> actually test it.
>>>>
>>>> Yes, I have attached the trace file which is Vishwa's one in fact.
>>> Thanks, I applied and pushed the patch.
>>>
>>> Please note that your trace has some suspicious tracepoints with same
>>> timestamps for end of cpuidle and start of next cpuidle.
>> That comes from a known problem about the timer on OMAP. The 32K timer
>> is feeding the trace timestamps and so the granularity is not high
>> enough (30us).
>
>>> Also, cpuidle states name in pytimechart are very selfishly hardcoded
>>> with intel's convention.
>>> Can you tell what is your convention, so that we can think of a best
>>> way to handle display of state's name for ftrace text output?
>> The power trace points API is described in 
>> Documentation/trace/events-power.txt.
>> i.e. trace_cpu_idle(4, smp_processor_id()) means that the system
>> enters the idle state 4, while trace_cpu_idle(PWR_EVENT_EXIT,
>> smp_processor_id())
> Yes, I know this.
>
> This state is depending on the cpuidle state table.
> on Intel arch, some CStates are reserved and unimplemented.
> thus the same of the Cstate does not correspond to the id of the cpuidle 
> state.
> e.g.
> 0 -> C0 (not an idle state, in IA)
> 1 -> C1
> 2 -> C2
> 3 -> C4
> 4 -> C6
> 5 -> S0i1 ( a platform idle state)
> 6 -> S0i3 ( another deeper platform idle state)
> This is reflected in the table (I just moved this table in cpuidle.py):
> c_state_table = ["C0","C1","C2","C4","C6","S0i1","S0i3"]
>
> So.. beware! cpuidle id 3 will currently be displayed as "C4" in
> pytimechart, which is what *intel_mid* guys want, but probably not
> what OMAP guys want.
Ok sorry I misunterstood the point.

> The cpuidle state names are usually available on sysfs. perf and
> trace-cmd are dumping this in their binary format, but ftrace text
> interface is not.
> So this has to be hardcoded or configured.
We could add the state name (or abbr) in the ftrace format but that
requires some generic kernel changes (again).

Thomas (added in the To: list) attempted to introduce some abbreviated
states names but I do not know if this went through to the mainline
and if it is applicable to pytimechart. Cf. [1] for the patch.

Thomas, can you help?

[1] http://marc.info/?l=linux-omap&m=129439625501174&w=2

Jean

>
> Regards,
> Pierre
>

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH] OMAP CPUIDLE: CPU Idle latency measurement

[Jean Pihet]

Benoit,

On Fri, Aug 27, 2010 at 12:25 PM, Cousson, Benoit  wrote:
>> This patch has instrumentation code for measuring latencies for
>> various CPUIdle C states for OMAP. Idea here is to capture the
>> timestamp at various phases of CPU Idle and then compute the sw
>> latency for various c states. For OMAP, 32k clock is chosen as
>> reference clock this as is an always on clock. wkup domain memory
>> (scratchpad memory) is used for storing timestamps. One can see the
>> worstcase latencies in below sysfs entries (after enabling
>> CONFIG_CPU_IDLE_PROF
>> in .config). This information can be used to correctly configure cpu idle
>> latencies for various C states after adding HW latencies for each of
>> these sw latencies.
>> /sys/devices/system/cpu/cpu0/cpuidle/state/actual_latency
>> /sys/devices/system/cpu/cpu0/cpuidle/state/sleep_latency
>> /sys/devices/system/cpu/cpu0/cpuidle/state/wkup_latency
>
> FYI, Jean is currently working on using standard Linux probes in order to
> instrument CPUIdle / CPUfreq. I'm not sure it is doable, but it might better
> to use standard method to do that instead of purely OMAP3 specific stuff.
> This code will not scale easily on OMAP4.
The proposed code looks OK to me since it is exporting the cpuidle
latency figures through the generic cpuidle driver (in
drivers/cpuidle/sysfs.c, via
/sys/devices/system/cpu/cpu0/cpuidle/state/).
Once that code gets approved I can (and will) add some trace points in
it, so that all PM related events can be traced vs time.


>
> Do you have a dump of the latency you measured do far?

Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH] OMAP CPUIDLE: CPU Idle latency measurement

[Jean Pihet]

Hi,

On Sat, Aug 28, 2010 at 12:08 AM,   wrote:
> From: Vishwanath BS 
>
> This patch has instrumentation code for measuring latencies for
> various CPUIdle C states for OMAP. Idea here is to capture the
> timestamp at various phases of CPU Idle and then compute the sw
> latency for various c states. For OMAP, 32k clock is chosen as
> reference clock this as is an always on clock. wkup domain memory
> (scratchpad memory) is used for storing timestamps. One can see the
> worstcase latencies in below sysfs entries (after enabling 
> CONFIG_CPU_IDLE_PROF
> in .config). This information can be used to correctly configure cpu idle
> latencies for various C states after adding HW latencies for each of
> these sw latencies.
> /sys/devices/system/cpu/cpu0/cpuidle/state/actual_latency
> /sys/devices/system/cpu/cpu0/cpuidle/state/sleep_latency
> /sys/devices/system/cpu/cpu0/cpuidle/state/wkup_latency
>
> THis patch is tested on OMAP ZOOM3 using kevin's pm branch.
>
> Signed-off-by: Vishwanath BS 
> Cc: linaro-dev@lists.linaro.org
> ---
...

>
> +#ifdef CONFIG_CPU_IDLE_PROF
> +       sleep_time = omap3_sram_get_sleep_time();
> +       wkup_time = omap3_sram_get_wkup_time();
> +
> +       /* take care of overflow */
> +       if (postidle_time < preidle_time)
> +               postidle_time += (u32) 0x;
> +       if (wkup_time < sleep_time)
> +               wkup_time += (u32) 0x;
> +
> +       idle_time = postidle_time - preidle_time;
> +       total_sleep_time = wkup_time -  sleep_time;
> +       latency = idle_time - total_sleep_time;
> +       sleep_time = omap3_sram_get_sleep_time();
> +       wkup_time = omap3_sram_get_wkup_time();
Is it needed to re-read the sleep_time and wkup_time values from the scratchpad?
What about the 32k timer overflow? Could that cause the sleep_latency
and/or wkup_latency to be <0?

> +
> +       /* calculate average latency after ignoring sprious ones */
> +       if ((total_sleep_time > 0) && (latency > state->actual_latency)
> +               && (latency >= 0)) {
> +               state->actual_latency = CONVERT_32K_USEC(latency);
> +               latency = (sleep_time - preidle_time);
Risk of overflow?

> +               state->sleep_latency = CONVERT_32K_USEC(latency);
> +               latency = postidle_time - wkup_time;
Risk of overflow?

> +               state->wkup_latency = CONVERT_32K_USEC(latency);
> +       }
> +#endif
> +
>        return ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * USEC_PER_SEC;
>  }
>

Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH] OMAP CPUIDLE: CPU Idle latency measurement

[Jean Pihet]

Hi Amit, Santosh,

On Thu, Sep 2, 2010 at 10:11 AM, Shilimkar, Santosh
 wrote:
...
>> > The point is to keep the minimum possible in the kernel: just the
>> > tracepoints we're interested in.   The rest (calculations, averages,
>> > analysis, etc.) does not need to be in the kernel and can be done easier
>> > and with more powerful tools outside the kernel.
>>
>> Kevin,
>>
>> I agree. We discussed this a little in our weekly meeting. Vishwa's main
>> concern was the lack of ability to instrument the last bit of SRAM code.
>>
>> I have a feeling that even with caches on when we enter this code, we
>> won't
>> see too much variance in the latency to execute this bit of code. Vishwa
>> is
>> going to confirm that now. If that hypothesis is true, we can indeed move
>> to
>> using tracepoints in the idle path and use external tools to track latency.
I agree. Can you confirm this asap?
I am looking at the instrumentation tracepoints now. I think it would
be ideal to provide multiple tracepoints in both sleep and wake-up
paths.

> There will be difference with and without caches but the delta latency will 
> be constant with caches and without caches. Another important point is
> he lowest level code should be just profiled once and for worst CPU/BUS clock 
> speed.
Ok.

>> Even if it isn't true, the rest of the idle path could still contain
>> tracepoints.
I am looking at the best way to introduce tracepoints in the low level
code, in case it is needed.

> I also think this would be better approach considering a generic solution.
Good!

>
> Regards,
> Santosh
>

Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: [PATCH] OMAP CPUIDLE: CPU Idle latency measurement

[Jean Pihet]

Hi Vishwa,

On Mon, Sep 6, 2010 at 1:15 PM, Sripathy, Vishwanath
 wrote:
> I did some profiling of assembly code on OMAP3630 board (ZOOM3). In worst 
> case it takes around 3.28ms and best case around 2.93ms for mpu off mode.
Can you give a bit more details? Which measurement has been taken (ASM
only, sleep, wake-up ...?) and what are the significant figures?

>For MPU INACTIVE/RET, it is less than 30us.
Mmh that is the resolution of the 32kHz timer, so I guess you get
either 0 or 1 timer cycle depending when you start the measurement.
IMO the 32kHz timer is too slow to measure those fast events.

> However as Kevin mentioned in other email, it would be better to find out a 
> way to trace inside assembly code as well.
OK that could be done but first I would like to make sure such a
complication is  needed.

>
> Regards
> Vishwa

Jean

___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev