Re: [PATCH] arm: Handle device tree memory regions larger than 4GB

[Peter Maydell]

On 6 July 2012 00:27, Rob Herring  wrote:
> On 07/05/2012 11:48 AM, Peter Maydell wrote:
>>  void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>>  {
>> + while (size > 0x8000) {
>> + arm_add_memory(base, 0x8000);
>> + base += 0x8000;
>> + size -= 0x8000;
>> + }
>>   arm_add_memory(base, size);
>
> I would just change arm_add_memory to use phys_addr_t for the size
> param. This ultimately calls memblock functions which use phys_addr_t
> for sizes.

I was wondering if somebody would suggest that. It's a bigger patch
(since it requires changing struct membank and potentially any function
which uses that) but I guess it is the long term right thing. I'll
have a go at that next week...

> One thing I noticed is ATAGs will be broken for LPAE. That's probably
> fine because if you can fix your bootloader for new ATAGs, then you can
> support DT.

Well, it works fine with LPAE assuming you didn't actually need to
pass in 4GB of RAM. (I think you could for instance specify 3.5GB
RAM starting at 2GB using ATAGS.) But yes, I'm assuming ATAGs boot
is 'legacy' now and we can insist on DT for any new platform that
can manage 4GB of RAM in the first place.

-- PMM

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

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> The 'enter_dead' function is only used for processor_idle.c
> and the same function is used several times. We fall into the
> same abuse with the multiple callbacks for the same function.

This isn't abuse, mind you.  This is a normal practice.

> This patch fixes that by moving the 'enter_dead' function to the
> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> to handle the callback conditional invokation.

And how does that improve things?

Rafael

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

Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> Move this specific flag to the header file.

The patch evidently does more than that.

Is it just a cleanup, or is there a functional reason for doing it?

Rafael


> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/idle/intel_idle.c |8 
>  include/linux/cpuidle.h   |   16 +---
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index fe95d54..3f0eb07 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu);
>  static struct cpuidle_state *cpuidle_state_table;
>  
>  /*
> - * Set this flag for states where the HW flushes the TLB for us
> - * and so we don't need cross-calls to keep it consistent.
> - * If this flag is set, SW flushes the TLB, so even if the
> - * HW doesn't do the flushing, this flag is safe to use.
> - */
> -#define CPUIDLE_FLAG_TLB_FLUSHED 0x1
> -
> -/*
>   * States are indexed by the cstate number,
>   * which is also the index into the MWAIT hint array.
>   * Thus C0 is a dummy.
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 730e12e..be150c9 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -67,10 +67,20 @@ struct cpuidle_state {
>   *the cpuidle core the specified state can use 
> the  *
>   *enter_dead function.   
>*
>   *   
>*
> + * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes 
> the *
> + *TLB for us and so we don't need cross-calls to 
>*
> + *keep it consistent. If this flag is set, SW
>*
> + *flushes the TLB, so even if the HW doesn't do 
> the *
> + *flushing, this flag is safe to use.
>*
> + *   
>*
>   
> ***/
> -#define CPUIDLE_FLAG_TIME_VALID  (0x01)
> -#define CPUIDLE_FLAG_COUPLED (0x02)
> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
> +#define CPUIDLE_FLAG_TIME_VALID  (0x01)
> +#define CPUIDLE_FLAG_COUPLED (0x02)
> +#define CPUIDLE_FLAG_DEAD_VALID  (0x04)
> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
> +
> +
> +
>  
>  /**
>   * cpuidle_get_statedata - retrieves private driver state data
> 


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

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Rafael J. Wysocki]

On Thursday, July 05, 2012, 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.

Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
Among other things, this was to avoid recurrence in callbacks - please see
acpi_idle_enter_bm() for example.

Now, if .enter() is moved to the driver structure, it will have to be an
all-purpose complicated routine calling itself recursively at least in
some cases.  I'm not quite convinced that would be an improvement.

On the other hand, I don't see anything wrong with setting several callback
pointers to the same routine.

Thanks,
Rafael

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

Re: [PATCH 1/4] acpi: intel_idle : break dependency between modules

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 
> Signed-off-by: Daniel Lezcano 
> Acked-by: Srivatsa S. Bhat 
> Acked-by: Rafael J. Wysocki 

OK, I'm taking this one.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_driver.c |7 --
>  drivers/idle/intel_idle.c   |   41 +-
>  include/linux/cpuidle.h |7 --
>  3 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 13103aeb..7048b97 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -429,18 +429,11 @@ static int acpi_cpu_soft_notify(struct notifier_block 
> *nfb,
>* Initialize missing things
>*/
>   if (pr->flags.need_hotplug_init) {
> - struct cpuidle_driver *idle_driver =
> - cpuidle_get_driver();
> -
>   printk(KERN_INFO "Will online and init hotplugged "
>  "CPU: %d\n", pr->id);
>   WARN(acpi_processor_start(pr), "Failed to start CPU:"
>   " %d\n", pr->id);
>   pr->flags.need_hotplug_init = 0;
> - if (idle_driver && !strcmp(idle_driver->name,
> -"intel_idle")) {
> - intel_idle_cpu_init(pr->id);
> - }
>   /* Normal CPU soft online event */
>   } else {
>   acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..fe95d54 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  static int intel_idle(struct cpuidle_device *dev,
>   struct cpuidle_driver *drv, int index);
> +static int intel_idle_cpu_init(int cpu);
>  
>  static struct cpuidle_state *cpuidle_state_table;
>  
> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>   clockevents_notify(reason, &cpu);
>  }
>  
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> - unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +   unsigned long action, void *hcpu)
>  {
>   int hotcpu = (unsigned long)hcpu;
> + struct cpuidle_device *dev;
>  
>   switch (action & 0xf) {
>   case CPU_ONLINE:
> - smp_call_function_single(hotcpu, __setup_broadcast_timer,
> - (void *)true, 1);
> +
> + if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
> + smp_call_function_single(hotcpu, 
> __setup_broadcast_timer,
> +  (void *)true, 1);
> +
> + /*
> +  * Some systems can hotplug a cpu at runtime after
> +  * the kernel has booted, we have to initialize the
> +  * driver in this case
> +  */
> + dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> + if (!dev->registered)
> + intel_idle_cpu_init(hotcpu);
> +
>   break;
>   }
>   return NOTIFY_OK;
>  }
>  
> -static struct notifier_block setup_broadcast_notifier = {
> - .notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> + .notifier_call = cpu_hotplug_notify,
>  };
>  
>  static void auto_demotion_disable(void *dummy)
> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
>  
>   if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
>   lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
> - else {
> + else
>   on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> - register_cpu_notifier(&setup_broadcast_notifier);
> - }
> +
> + register_cpu_notifier(&cpu_hotplug_notifier);
>  
>   pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>   " model 0x%X\n", boot_cpu_data.x86_model);
> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>   int 

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, 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.
> 
> Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> Among other things, this was to avoid recurrence in callbacks - please see
> acpi_idle_enter_bm() for example.
> 
> Now, if .enter() is moved to the driver structure, it will have to be an
> all-purpose complicated routine calling itself recursively at least in
> some cases.  I'm not quite convinced that would be an improvement.
> 
> On the other hand, I don't see anything wrong with setting several callback
> pointers to the same routine.

Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
to a single structure stored in the driver. The drivers were modified
and cleanup to take into account this modification.

We saw some regressions like for example the 'disable' which were not
per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
workaround more than a real fix). And now we have new architectures
(tegra3, big.LITTLE) with different latencies per cpu. Logically we
should revert Deepthi's patches but from my POV, going back and forth is
not a good solution (also we have to undo all modifications done in the
different drivers).

The main purpose of all these cleanup patches are to move out all
non-data information from the cpuidle_state structure in order to add a
new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
cpuidle_latencies latencies)'.

For this specific patch, the 'enter' function for all the drivers is not
used [1] and one of the driver is about to use a single function [2]. So
we have only one driver is a couple of functions for this which can be
replaced by an array of callbacks in the driver itself as we have the index.

[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
[2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


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

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Daniel Lezcano]

On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> The 'enter_dead' function is only used for processor_idle.c
>> and the same function is used several times. We fall into the
>> same abuse with the multiple callbacks for the same function.
> 
> This isn't abuse, mind you.  This is a normal practice.

Well, that depends :)

I agree adding a callback per state is nice and flexible but if it is
not used, it is a waste of memory, even if it is 32 bytes.

>> This patch fixes that by moving the 'enter_dead' function to the
>> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
>> to handle the callback conditional invokation.
> 
> And how does that improve things?

In order to check if the play_dead is enabled for a specific state, we
check if the pointer is set. As it has been moved to a single function,
we need to add a flag to replace this check. That is not an improvement,
it replace a check by another check.


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


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

Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Daniel Lezcano]

On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> Move this specific flag to the header file.
> 
> The patch evidently does more than that.
> 
> Is it just a cleanup, or is there a functional reason for doing it?

It is just a cleanup.


Thanks for reviewing the patches.

  -- Daniel


>> Signed-off-by: Daniel Lezcano 
>> ---
>>  drivers/idle/intel_idle.c |8 
>>  include/linux/cpuidle.h   |   16 +---
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index fe95d54..3f0eb07 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu);
>>  static struct cpuidle_state *cpuidle_state_table;
>>  
>>  /*
>> - * Set this flag for states where the HW flushes the TLB for us
>> - * and so we don't need cross-calls to keep it consistent.
>> - * If this flag is set, SW flushes the TLB, so even if the
>> - * HW doesn't do the flushing, this flag is safe to use.
>> - */
>> -#define CPUIDLE_FLAG_TLB_FLUSHED0x1
>> -
>> -/*
>>   * States are indexed by the cstate number,
>>   * which is also the index into the MWAIT hint array.
>>   * Thus C0 is a dummy.
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 730e12e..be150c9 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -67,10 +67,20 @@ struct cpuidle_state {
>>   *the cpuidle core the specified state can use 
>> the  *
>>   *enter_dead function.  
>> *
>>   *  
>> *
>> + * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes 
>> the *
>> + *TLB for us and so we don't need cross-calls 
>> to*
>> + *keep it consistent. If this flag is set, SW   
>> *
>> + *flushes the TLB, so even if the HW doesn't do 
>> the *
>> + *flushing, this flag is safe to use.   
>> *
>> + *  
>> *
>>   
>> ***/
>> -#define CPUIDLE_FLAG_TIME_VALID (0x01)
>> -#define CPUIDLE_FLAG_COUPLED(0x02)
>> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
>> +#define CPUIDLE_FLAG_TIME_VALID  (0x01)
>> +#define CPUIDLE_FLAG_COUPLED (0x02)
>> +#define CPUIDLE_FLAG_DEAD_VALID  (0x04)
>> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
>> +
>> +
>> +
>>  
>>  /**
>>   * cpuidle_get_statedata - retrieves private driver state data
>>
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


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

Re: [PATCH] arm: Handle device tree memory regions larger than 4GB

[Peter Maydell]

On 6 July 2012 00:27, Rob Herring  wrote:
> I would just change arm_add_memory to use phys_addr_t for the size
> param. This ultimately calls memblock functions which use phys_addr_t
> for sizes.

So I have a patch that does this which basically works. However
there is a bit I'm not sure about. arm_add_memory() does this:
   bank->size = size & PAGE_MASK;

in an attempt to clear the bottom bits of the size. However,
since PAGE_MASK is defined as:
 #define PAGE_SIZE   (_AC(1,UL) << PAGE_SHIFT)
 #define PAGE_MASK   (~(PAGE_SIZE-1))

PAGE_MASK is a 32 bit unsigned constant and so this & will
clear the top 32 bits of bank->size.

I'm really not sure what the best way to fix this is; suggestions?

thanks
-- PMM

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

Re: [PATCH] arm: Handle device tree memory regions larger than 4GB

[Dave Martin]

On Fri, Jul 06, 2012 at 07:07:35PM +0100, Peter Maydell wrote:
> On 6 July 2012 00:27, Rob Herring  wrote:
> > I would just change arm_add_memory to use phys_addr_t for the size
> > param. This ultimately calls memblock functions which use phys_addr_t
> > for sizes.
> 
> So I have a patch that does this which basically works. However
> there is a bit I'm not sure about. arm_add_memory() does this:
>bank->size = size & PAGE_MASK;
> 
> in an attempt to clear the bottom bits of the size. However,
> since PAGE_MASK is defined as:
>  #define PAGE_SIZE   (_AC(1,UL) << PAGE_SHIFT)
>  #define PAGE_MASK   (~(PAGE_SIZE-1))
> 
> PAGE_MASK is a 32 bit unsigned constant and so this & will
> clear the top 32 bits of bank->size.
> 
> I'm really not sure what the best way to fix this is; suggestions?

Maybe something like

~(phys_addr_t)~PAGE_MASK

or

~(phys_addr_t)(PAGE_SIZE - 1)

would work.

---Dave

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

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, 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.
> > 
> > Hmm. I suppose the cpuidle subsystem was designed the way it was for a 
> > reason.
> > Among other things, this was to avoid recurrence in callbacks - please see
> > acpi_idle_enter_bm() for example.
> > 
> > Now, if .enter() is moved to the driver structure, it will have to be an
> > all-purpose complicated routine calling itself recursively at least in
> > some cases.  I'm not quite convinced that would be an improvement.
> > 
> > On the other hand, I don't see anything wrong with setting several callback
> > pointers to the same routine.
> 
> Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
> to a single structure stored in the driver. The drivers were modified
> and cleanup to take into account this modification.
> 
> We saw some regressions like for example the 'disable' which were not
> per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
> workaround more than a real fix). And now we have new architectures
> (tegra3, big.LITTLE) with different latencies per cpu. Logically we
> should revert Deepthi's patches but from my POV, going back and forth is
> not a good solution (also we have to undo all modifications done in the
> different drivers).
> 
> The main purpose of all these cleanup patches are to move out all
> non-data information from the cpuidle_state structure in order to add a
> new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
> cpuidle_latencies latencies)'.

Well, my question is: what specifically is wrong with having callback
pointers in struct cpuidle_state in case when that structure is known
to be common for all the CPUs in the system?

Sure, there are systems in which that is not the case, but then why
should we complicate the simplest case in order to address them?

> For this specific patch, the 'enter' function for all the drivers is not
> used [1] and one of the driver is about to use a single function [2]. So
> we have only one driver is a couple of functions for this which can be
> replaced by an array of callbacks in the driver itself as we have the index.
> 
> [1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html

I agree with Deepthi.

> [2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html

And that is fine and dandy as long as the OMAP people want that.  Evidently,
that is not wanted by everybody and if it had been, the cpuidle subsystem
would have been designed differently.

I really would like that thing to be reconsidered before we proceed with
these changes.

Thanks,
Rafael

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

Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> Move this specific flag to the header file.
> > 
> > The patch evidently does more than that.
> > 
> > Is it just a cleanup, or is there a functional reason for doing it?
> 
> It is just a cleanup.

In that case, can you please say that in the changelog?  I have no objections
to the patch itself, but the changelog is clearly substandard.

> Thanks for reviewing the patches.

No problem.

Thanks,
Rafael

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

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> The 'enter_dead' function is only used for processor_idle.c
> >> and the same function is used several times. We fall into the
> >> same abuse with the multiple callbacks for the same function.
> > 
> > This isn't abuse, mind you.  This is a normal practice.
> 
> Well, that depends :)
> 
> I agree adding a callback per state is nice and flexible

Yes, it is.

> but if it is not used, it is a waste of memory, even if it is 32 bytes.

32 bits, perhaps?  And how many of those are there in the whole system,
actually?  Is this a number that actually matters?

> >> This patch fixes that by moving the 'enter_dead' function to the
> >> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> >> to handle the callback conditional invokation.
> > 
> > And how does that improve things?
> 
> In order to check if the play_dead is enabled for a specific state, we
> check if the pointer is set. As it has been moved to a single function,
> we need to add a flag to replace this check. That is not an improvement,

Well, exactly.

> it replace a check by another check.

Sorry, but I don't really see the point.

Thanks,
Rafael

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