Re: [RFC PATCH 05/17] ARM: kernel: save/restore kernel IF

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:50:18PM +0100, Lorenzo Pieralisi wrote:
> +static int late_init(void)
> +{
> + int rc;
> + struct sr_cluster *cluster;
> + int cluster_index, cpu_index = sr_platform_get_cpu_index();

Stop this madness, and use the standard linux APIs like smp_processor_id
here.  It might actually help you find bugs, like the fact that you're
in a preemptible context here, and so could be rescheduled onto any other
CPU in the system _after_ you've read the MPIDR register.

> +
> + cluster_index = sr_platform_get_cluster_index();
> + cluster = main_table.cluster_table + cluster_index;
> + main_table.os_mmu_context[cluster_index][cpu_index] =
> + current->active_mm->pgd;
> + cpu_switch_mm(main_table.fw_mmu_context, current->active_mm);
> + rc = sr_platform_init();
> + cpu_switch_mm(main_table.os_mmu_context[cluster_index][cpu_index],
> + current->active_mm);

CPU numbers are unique in the system, why do you need a 'cluster_index'
to save this?  In fact why do you even need to save it in a structure
at all?

Plus, "cluster" is not used, please get rid of it.

Plus, here you just switch the page tables without a MMU flush.  Further
down in this file you call cpu_switch_mm() but also flush the TLB.  Why
that difference?

If this is the state of just the first bit of this I've looked at, plus
the comments from Frank on your use of internal functions like
cpu_do_suspend and cpu_do_resume, I don't want to look at it any further.
Can you please clean it up as best you can first.

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

Re: [RFC PATCH 05/17] ARM: kernel: save/restore kernel IF

[Russell King - ARM Linux]

On Sat, Jul 09, 2011 at 09:38:15AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 07, 2011 at 04:50:18PM +0100, Lorenzo Pieralisi wrote:
> > +static int late_init(void)
> > +{
> > +   int rc;
> > +   struct sr_cluster *cluster;
> > +   int cluster_index, cpu_index = sr_platform_get_cpu_index();
> 
> Stop this madness, and use the standard linux APIs like smp_processor_id
> here.  It might actually help you find bugs, like the fact that you're
> in a preemptible context here, and so could be rescheduled onto any other
> CPU in the system _after_ you've read the MPIDR register.
> 
> > +
> > +   cluster_index = sr_platform_get_cluster_index();
> > +   cluster = main_table.cluster_table + cluster_index;
> > +   main_table.os_mmu_context[cluster_index][cpu_index] =
> > +   current->active_mm->pgd;
> > +   cpu_switch_mm(main_table.fw_mmu_context, current->active_mm);
> > +   rc = sr_platform_init();
> > +   cpu_switch_mm(main_table.os_mmu_context[cluster_index][cpu_index],
> > +   current->active_mm);
> 
> CPU numbers are unique in the system, why do you need a 'cluster_index'
> to save this?  In fact why do you even need to save it in a structure
> at all?
> 
> Plus, "cluster" is not used, please get rid of it.

Oh, that's another thing.  This thread is about introducing idle support
not cluster support.  Cluster support is surely something different (esp.
as it seems from the above code that you're trying to support several
clusters of MPCore CPUs, each with physical 0-N CPU numbers.)

Cluster support should be an entirely separate patch series.

If that is not what this cluster stuff in this patch is about, then it's
just written badly and reinforces the need for it to be rewritten - in
that case there's no need for a 2D array.

In any case, smp_processor_id() will be (and must be) unique in any given
running kernel across all CPUs, even if you have clusters of N CPUs all
physically numbered 0-N.

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

Re: [RFC PATCH 06/17] ARM: kernel: save/restore generic infrastructure

[Russell King - ARM Linux]

The idea of splitting a large patch up into smaller patches is to do
it in a logical way so that:

1. Each patch is self-contained, adding a single new - and where possible
   complete - feature or bug fix.
2. Ease of review.

Carving your big patch up by file does not do either of this, because
all patches interact with each other.  There's people within ARM Ltd who
have been dealing with patch sets for some time who you could ask advice
from - or who you could ask to review your patches before you send them
out on the mailing list.

On Thu, Jul 07, 2011 at 04:50:19PM +0100, Lorenzo Pieralisi wrote:
> diff --git a/arch/arm/kernel/sr.h b/arch/arm/kernel/sr.h
> new file mode 100644
> index 000..6b24e53
> --- /dev/null
> +++ b/arch/arm/kernel/sr.h
> @@ -0,0 +1,162 @@
> +#define SR_NR_CLUSTERS 1
> +
> +#define STACK_SIZE 512
> +
> +#define CPU_A5   0x4100c050

Not used in this patch - should it be in some other patch?

> +#define CPU_A8   0x4100c080

Not used in this patch - should it be in some other patch?

> +#define CPU_A9   0x410fc090

Not used in this patch - and if this is generic code then it should not
be specific to any CPU.  Please get rid of all these definitions, and
if they're used you need to rework these patches to have proper separation
of generic code from CPU specific code.

> +#define L2_DATA_SIZE 16

Not used in this patch.

> +#define CONTEXT_SPACE_UNCACHED   (2 * PAGE_SIZE)
> +#define PA(f) ((typeof(f) *)virt_to_phys((void *)f))

This is just messed up.  Physical addresses aren't pointers, they're
numeric.  PA() is not used in this patch either, so it appears it can
be deleted.

> +extern int lookup_arch(void);

This doesn't exist in this patch, should it be in another patch or
deleted?

> +/*
> + * Global variables
> + */
> +extern struct sr_main_table main_table;
> +extern unsigned long idle_save_context;
> +extern unsigned long idle_restore_context;
> +extern unsigned long idle_mt;
> +extern void *context_memory_uncached;

Why does this need to be a global?

> +
> +/*
> + * Context save/restore
> + */
> +typedef u32 (sr_save_context_t)
> + (struct sr_cluster *,
> + struct sr_cpu*, u32);

Fits on one line so should be on one line.

> +typedef u32 (sr_restore_context_t)
> + (struct sr_cluster *,
> + struct sr_cpu*);

Fits on one line so should be on one line.

> +
> +extern sr_save_context_t sr_save_context;

This doesn't exist in this patch, should it be in another patch?

> +extern sr_restore_context_t sr_restore_context;

Ditto.

> +
> +
> +extern struct sr_arch *get_arch(void);

Ditto.

> +
> +
> +/*
> + * 1:1 mappings
> + */
> +
> +extern int linux_sr_setup_translation_tables(void);

Ditto.

> +
> +/*
> + * dumb memory allocator
> + */
> +
> +extern void *get_memory(unsigned int size);
> +
> +/*
> + * Entering/Leaving C-states function entries
> + */
> +
> +extern int sr_platform_enter_cstate(unsigned cpu_index, struct sr_cpu *cpu,
> + struct sr_cluster *cluster);
> +extern int sr_platform_leave_cstate(unsigned cpu_index, struct sr_cpu *cpu,
> + struct sr_cluster *cluster);

See comment at the bottom - would inline functions here be better
or maybe even place them at the callsite to make the code easier to
understand if they're only used at one location.

> +
> +/* save/restore main table */
> +extern struct sr_main_table main_table;

Why do we have two 'main_table' declarations in this same header file?

> +
> +/*
> + * Init functions
> + */
> +
> +extern int sr_platform_runtime_init(void);

Not defined in this patch.

> +extern int sr_platform_init(void);
> +extern int sr_context_init(void);
> +
> +
> +/*
> + * v7 specific
> + */
> +
> +extern char *cpu_v7_suspend_size;

No - stop going underneath the covers of existing APIs to get what you
want.  Use cpu_suspend() and cpu_resume() directly.  Note that they've
changed to be more flexible and those patches have been on this mailing
list, and will be going in for 3.1.

If they still don't do what you need, I'm going to be *pissed* because
you've obviously known that they don't yet you haven't taken the time
to get involved on this mailing list with the discussions over it.

> +extern void scu_cpu_mode(void __iomem *base, int state);

Not defined in this patch - should it be in another patch or deleted?

> +
> +/*
> + * These arrays keep suitable stack pointers for CPUs.
> + *
> + * The memory must be 8-byte aligned.
> + */
> +
> +extern unsigned long platform_cpu_stacks[CONFIG_NR_CPUS];

Ditto.

> +extern unsigned long platform_cpu_nc_stacks[CONFIG_NR_CPUS];

Ditto.

And should these be per-cpu variables?  In any case, CONFIG_NR_CPUS
doesn't look right here, NR_CPUS is probably what you want.

> +#endif
> diff --git a/arch/arm/kernel/sr_context.c b/arch/arm/kernel/sr_context.c
> new file mode 100644
> index 000..25eaa43
> --- /dev/null
> +++ b/arch/arm/kernel/sr_context.c
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright 

Re: [PATCH 01/17] ARM: proc: add definition of cpu_reset for ARMv6 and ARMv7 cores

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:50:14PM +0100, Lorenzo Pieralisi wrote:
> From: Will Deacon 
> 
> This patch adds simple definitions of cpu_reset for ARMv6 and ARMv7
> cores, which disable the MMU via the SCTLR.

This really needs fixing properly, so that we have this well defined
across all supported ARM cores.  Requiring ARMv6 and ARMv7 to have this
code called with a flat mapping (which may overlap a section boundary)
vs ARMv5 and lower code which doesn't is just silly.

With any API, we need consistency.  So if ARMv6 and v7 require a flat
mapping, we need to ensure that ARMv5 and lower is happy to deal with
that code being also called with a flat mapping.

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

Re: [Patch 5/11] Regulator: DA9052 regulator support v2

[Mark Brown]

On Wed, Jul 06, 2011 at 04:19:27PM +0530, Ashish Jangam wrote:

> > > The DA9052 PMIC has below featured regulators:-

> > Having patch version info as [Patch V2 5/11] would be better.

> This is a good idea but in my recent post I did not incorporate it
> bcoz this would change the subject line and identifying the mail threads
> will become a problem. 

Given that you're not even threading patches within a series this is
hardly an issue, though in any case sending new patches as replies to
old versions is normally a bad idea as it makes it hard to find new
versions in archives and it can become extremely confusing trying to
work out what the eurrent series looks like if the discussion is long or
involved.

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

Re: [PATCH 02/17] ARM: Add cpu power management notifiers

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:50:15PM +0100, Lorenzo Pieralisi wrote:
> During some CPU power modes entered during idle, hotplug and
> suspend, peripherals located in the CPU power domain, such as
> the GIC and VFP, may be powered down.  Add a notifier chain
> that allows drivers for those peripherals to be notified
> before and after they may be reset.

I defer comment on this until I see what the CPU_COMPLEX_* stuff is used
for.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:50:16PM +0100, Lorenzo Pieralisi wrote:
> From: Colin Cross 
> 
> When the cpu is powered down in a low power mode, the gic cpu
> interface may be reset, and when the cpu complex is powered
> down, the gic distributor may also be reset.
> 
> This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save
> and restore the gic cpu interface registers, and the
> CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save
> and restore the gic distributor registers.
> 
> Signed-off-by: Colin Cross 

Lost attributations and original author.  This is based in part on code
from Gary King, according to patch 6646/1 in the patch system.

Moreover, how now that we have genirq dealing with the suspend/restore
issues, how much of this is actually required.  And should this be
GIC specific or should there be a way of asking genirq to take care of
some of this for us?

We need to _reduce_ the amount of code needed to support this stuff, and
if core code almost-but-not-quite does what we need then we need to talk
to the maintainers of that code to see whether it can be changed.

Because adding 212 lines to save and restore the state of every interrupt
controller that we may have just isn't on.  We need this properly
abstracted and dealt with in a generic way.

> ---
>  arch/arm/common/gic.c |  212 
> +
>  1 files changed, 212 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 4ddd0a6..8d62e07 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -42,6 +43,17 @@ struct gic_chip_data {
>   unsigned int irq_offset;
>   void __iomem *dist_base;
>   void __iomem *cpu_base;
> +#ifdef CONFIG_CPU_PM
> + u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
> + u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
> + u32 saved_spi_pri[DIV_ROUND_UP(1020, 4)];
> + u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
> + u32 __percpu *saved_ppi_enable;
> + u32 __percpu *saved_ppi_conf;
> + u32 __percpu *saved_ppi_pri;
> +#endif
> +
> + unsigned int gic_irqs;
>  };
>  
>  /*
> @@ -283,6 +295,8 @@ static void __init gic_dist_init(struct gic_chip_data 
> *gic,
>   if (gic_irqs > 1020)
>   gic_irqs = 1020;
>  
> + gic->gic_irqs = gic_irqs;
> +
>   /*
>* Set all global interrupts to be level triggered, active low.
>*/
> @@ -350,6 +364,203 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data 
> *gic)
>   writel_relaxed(1, base + GIC_CPU_CTRL);
>  }
>  
> +#ifdef CONFIG_CPU_PM
> +/*
> + * Saves the GIC distributor registers during suspend or idle.  Must be 
> called
> + * with interrupts disabled but before powering down the GIC.  After calling
> + * this function, no interrupts will be delivered by the GIC, and another
> + * platform-specific wakeup source must be enabled.
> + */
> +static void gic_dist_save(unsigned int gic_nr)
> +{
> + unsigned int gic_irqs;
> + void __iomem *dist_base;
> + int i;
> +
> + if (gic_nr >= MAX_GIC_NR)
> + BUG();
> +
> + gic_irqs = gic_data[gic_nr].gic_irqs;
> + dist_base = gic_data[gic_nr].dist_base;
> +
> + if (!dist_base)
> + return;
> +
> + for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
> + gic_data[gic_nr].saved_spi_conf[i] =
> + readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
> +
> + for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
> + gic_data[gic_nr].saved_spi_pri[i] =
> + readl_relaxed(dist_base + GIC_DIST_PRI + i * 4);
> +
> + for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
> + gic_data[gic_nr].saved_spi_target[i] =
> + readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
> +
> + for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
> + gic_data[gic_nr].saved_spi_enable[i] =
> + readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
> +
> + writel_relaxed(0, dist_base + GIC_DIST_CTRL);
> +}
> +
> +/*
> + * Restores the GIC distributor registers during resume or when coming out of
> + * idle.  Must be called before enabling interrupts.  If a level interrupt
> + * that occured while the GIC was suspended is still present, it will be
> + * handled normally, but any edge interrupts that occured will not be seen by
> + * the GIC and need to be handled by the platform-specific wakeup source.
> + */
> +static void gic_dist_restore(unsigned int gic_nr)
> +{
> + unsigned int gic_irqs;
> + unsigned int i;
> + void __iomem *dist_base;
> +
> + if (gic_nr >= MAX_GIC_NR)
> + BUG();
> +
> + gic_irqs = gic_data[gic_nr].gic_irqs;
> + dist_base = gic_data[gic_nr].dist_base;
> +
> + if (!dist_base)
> + return;
> +
> + writel_relaxed(0, dist_base + GIC_DI

Re: [PATCH 1/2] drivers: create a pinmux subsystem v3

[Mark Brown]

On Mon, Jun 13, 2011 at 01:57:36PM -0600, Grant Likely wrote:
> On Mon, Jun 13, 2011 at 10:58 AM, Linus Walleij

> > +This get/enable/disable/put sequence can just as well be handled by bus 
> > drivers
> > +if you don't want each and every driver to handle it and you know the
> > +arrangement on your bus.

> I would *strongly* recommend against individual device drivers
> accessing the pinmux api.  This is system level configuration code,
> and should be handled at the system level.

There can also be advantages to putting the pin into the designed mode
without the driver being loaded from the electrical point of view.  For
example, selecting appropriate pull values for pads can cut down on
power consumption.

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

Re: [PATCH 04/17] ARM: vfp: Use cpu pm notifiers to save vfp state

[Russell King - ARM Linux]

On Thu, Jul 07, 2011 at 04:50:17PM +0100, Lorenzo Pieralisi wrote:
> From: Colin Cross 
> 
> When the cpu is powered down in a low power mode, the vfp
> registers may be reset.
> 
> This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save
> and restore the cpu's vfp registers.
> 
> Signed-off-by: Colin Cross 
> ---
>  arch/arm/vfp/vfpmodule.c |   40 
>  1 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index f25e7ec..6f08dbe 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "vfpinstr.h"
>  #include "vfp.h"
> @@ -169,6 +170,44 @@ static struct notifier_block vfp_notifier_block = {
>   .notifier_call  = vfp_notifier,
>  };
>  
> +#ifdef CONFIG_CPU_PM
> +static int vfp_cpu_pm_notifier(struct notifier_block *self, unsigned long 
> cmd,
> + void *v)
> +{
> + u32 fpexc = fmrx(FPEXC);
> + unsigned int cpu = smp_processor_id();
> +
> + switch (cmd) {
> + case CPU_PM_ENTER:
> + if (last_VFP_context[cpu]) {
> + fmxr(FPEXC, fpexc | FPEXC_EN);
> + vfp_save_state(last_VFP_context[cpu], fpexc);
> + /* force a reload when coming back from idle */
> + last_VFP_context[cpu] = NULL;
> + fmxr(FPEXC, fpexc & ~FPEXC_EN);
> + }
> + break;

This doesn't look right.  On SMP setups, we always save the state of an
enabled VFP on thread switches.  That means the saved context in every
thread is always up to date for all threads, except _possibly_ for the
currently executing thread on the CPU.

On UP setups, we only save the state when we need to, so we need to do
something like the above.

However, we're growing more and more functions in the VFP code dealing
with saving and restoring state, and it's starting to become really silly
and confusing about which function is called for what and why, and why
it's different from another function doing something similar.

We need to sort this out so we have a _sane_ approach to this, rather
than inventing more and more creative ways to save VFP state and
restore it later.

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

Re: [PATCH 04/17] ARM: vfp: Use cpu pm notifiers to save vfp state

[Russell King - ARM Linux]

On Sat, Jul 09, 2011 at 11:44:08AM +0100, Russell King - ARM Linux wrote:
> We need to sort this out so we have a _sane_ approach to this, rather
> than inventing more and more creative ways to save VFP state and
> restore it later.

And here, let's prove that the current code is just soo bloody complex
that it needs redoing.  In the following code, 'last_VFP_context' is
renamed to 'vfp_current_hw_state' for clarity.

void vfp_sync_hwstate(struct thread_info *thread)
{
unsigned int cpu = get_cpu();

/*
 * If the thread we're interested in is the current owner of the
 * hardware VFP state, then we need to save its state.
 */
if (vfp_current_hw_state[cpu] == &thread->vfpstate) {
u32 fpexc = fmrx(FPEXC);

/*
 * Save the last VFP state on this CPU.
 */
fmxr(FPEXC, fpexc | FPEXC_EN);
vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
fmxr(FPEXC, fpexc);
}

Here, 'thread' is the thread we're interested in ensuring that we have
up to date context in thread->vfpstate.  On entry to this function, we
can be running on any CPU in the system, and 'thread' could have been
running on any other CPU in the system.

What this code is saying is: if the currrent CPU's hardware VFP state was
owned by this thread, then update the current VFP state.  So far it looks
sane.

Now, lets consider what with thread migration.  First, lets define three
threads.

Thread 1, we'll call 'interesting_thread' which is a thread which is
running on CPU0, using VFP (so vfp_current_hw_state[0] =
&interesting_thread->vfpstate) and gets migrated off to CPU1, where
it continues execution of VFP instructions.

Thread 2, we'll call 'new_cpu0_thread' which is the thread which takes
over on CPU0.  This has also been using VFP, and last used VFP on CPU0,
but doesn't use it again.

The following code will be executed twice:

cpu = thread->cpu;

/*
 * On SMP, if VFP is enabled, save the old state in
 * case the thread migrates to a different CPU. The
 * restoring is done lazily.
 */
if ((fpexc & FPEXC_EN) && vfp_current_hw_state[cpu]) {
vfp_save_state(vfp_current_hw_state[cpu], fpexc);
vfp_current_hw_state[cpu]->hard.cpu = cpu;
}
/*
 * Thread migration, just force the reloading of the
 * state on the new CPU in case the VFP registers
 * contain stale data.
 */
if (thread->vfpstate.hard.cpu != cpu)
vfp_current_hw_state[cpu] = NULL;

The first execution will be on CPU0 to switch away from 'interesting_thread'.
interesting_thread->cpu will be 0.

So, vfp_current_hw_state[0] points at interesting_thread->vfpstate.
The hardware state will be saved, along with the CPU number (0) that
it was executing on.

'thread' will be 'new_cpu0_thread' with new_cpu0_thread->cpu = 0.
Also, because it was executing on CPU0, new_cpu0_thread->vfpstate.hard.cpu = 0,
and so the thread migration check is not triggered.

This means that vfp_current_hw_state[0] remains pointing at interesting_thread.

The second execution will be on CPU1 to switch _to_ 'interesting_thread'.
So, 'thread' will be 'interesting_thread' and interesting_thread->cpu now
will be 1.  The previous thread executing on CPU1 is not relevant to this
so we shall ignore that.

We get to the thread migration check.  Here, we discover that
interesting_thread->vfpstate.hard.cpu = 0, yet interesting_thread->cpu is
now 1, indicating thread migration.  We set vfp_current_hw_state[1] to
NULL.

So, at this point vfp_current_hw_state[] contains the following:

[0] = interesting_thread
[1] = NULL

Our interesting thread now executes a VFP instruction, takes a fault
which loads the state into the VFP hardware.  Now, through the assembly
we now have:

[0] = interesting_thread
[1] = interesting_thread

CPU1 stops due to ptrace (and so saves its VFP state) using the thread
switch code above), and CPU0 calls vfp_sync_hwstate().

if (vfp_current_hw_state[cpu] == &thread->vfpstate) {
vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);

BANG, we corrupt interesting_thread's VFP state by overwriting the
more up-to-date state saved by CPU1 with the old VFP state from CPU0.

I think this is not the only problem with this code, and it's in
desperate need of being cleaned up.  Until such time, adding more
state saving code is just going to be extremely hairy.

Finally, as far as saving state for _idle_ goes (in other words, while
the CPU's idle loop in cpu_idle() is running), take a moment to consider
the following: the idle thread being a kernel thread does not use VFP.
It has no useful VFP state.  So:

1. On SMP, because we've switched away from any user

[uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David A. Long]

From: David A. Long 

Add a new "fdt_high" enviroment variable. This can be used to control (or 
prevent) the
relocation of the flattened device tree on boot. It can be used to prevent 
relocation
of the fdt into highmem.  The variable behaves similarly to the existing 
"initrd_high"
variable.

Signed-off-by: David A. Long 
---
 README |9 
 common/image.c |   60 ++--
 2 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/README b/README
index 8bb9c8d..5b95246 100644
--- a/README
+++ b/README
@@ -3281,6 +3281,15 @@ List of environment variables (most likely not complete):
  This can be used to load and uncompress arbitrary
  data.
 
+  fdt_high - if set this restricts the maximum address that the
+ flattened device tree will be copied into upon boot.
+ If this is set to the special value 0x then
+ the fdt will not be copied at all on boot.  For this
+ to work it must reside in writable memory, have
+ sufficient padding on the end of it for u-boot to
+ add the information it needs into it, and the memory
+ must be accessible by the kernel.
+
   i2cfast  - (PPC405GP|PPC405EP only)
  if set to 'y' configures Linux I2C driver for fast
  mode (400kHZ). This environment variable is used in
diff --git a/common/image.c b/common/image.c
index e542a57..7853de0 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1234,8 +1234,10 @@ int boot_relocate_fdt (struct lmb *lmb, char 
**of_flat_tree, ulong *of_size)
 {
void*fdt_blob = *of_flat_tree;
void*of_start = 0;
+   char*fdt_high;
ulong   of_len = 0;
int err;
+   int disable_relocation=0;
 
/* nothing to do */
if (*of_size == 0)
@@ -1249,26 +1251,62 @@ int boot_relocate_fdt (struct lmb *lmb, char 
**of_flat_tree, ulong *of_size)
/* position on a 4K boundary before the alloc_current */
/* Pad the FDT by a specified amount */
of_len = *of_size + CONFIG_SYS_FDT_PAD;
-   of_start = (void *)(unsigned long)lmb_alloc_base(lmb, of_len, 0x1000,
-   getenv_bootm_mapsize() + getenv_bootm_low());
+
+   /* If fdt_high is set use it to select the relocation address */
+   fdt_high = getenv("fdt_high");
+   if (fdt_high) {
+   void *desired_addr = (void *)simple_strtoul(fdt_high, NULL, 16);
+
+   if (((ulong) desired_addr) == ~0UL) {
+   /* All ones means use fdt in place */
+   desired_addr = fdt_blob;
+   disable_relocation = 1;
+   }
+   if (desired_addr) {
+   of_start =
+   (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
+  ((ulong)
+   desired_addr)
+  + of_len);
+   if (desired_addr && of_start != desired_addr) {
+   puts("Failed using fdt_high value for Device 
Tree");
+   goto error;
+   }
+   } else {
+   of_start =
+   (void *)(ulong) mb_alloc(lmb, of_len, 0x1000);
+   }
+   } else {
+   of_start =
+   (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
+  getenv_bootm_mapsize()
+  + getenv_bootm_low());
+   }
 
if (of_start == 0) {
puts("device tree - allocation error\n");
goto error;
}
 
-   debug ("## device tree at %p ... %p (len=%ld [0x%lX])\n",
-   fdt_blob, fdt_blob + *of_size - 1, of_len, of_len);
+   if (disable_relocation) {
+   /* We assume there is space after the existing fdt to use for 
padding */
+   fdt_set_totalsize(of_start, of_len);
+   printf("   Using Device Tree in place at %p, end %p\n",
+  of_start, of_start + of_len - 1);
+   } else {
+   debug ("## device tree at %p ... %p (len=%ld [0x%lX])\n",
+   fdt_blob, fdt_blob + *of_size - 1, of_len, of_len);
 
-   printf ("   Loading Device Tree to %p, end %p ... ",
-   of_start, of_start + of_len - 1);
+   printf ("   Loading Device Tree to %p, end %p ... ",
+   of_start, of_start + of_len - 1);
 
-   err = fdt_open_into (fdt_blob, of_start, of_len);
-   if (err != 0) {
-   fdt_error ("fdt move failed");
-   goto error;
+   err = fdt_open_into (fdt

Re: [PATCH 02/17] ARM: Add cpu power management notifiers

[Colin Cross]

On Sat, Jul 9, 2011 at 3:15 AM, Russell King - ARM Linux
 wrote:
> On Thu, Jul 07, 2011 at 04:50:15PM +0100, Lorenzo Pieralisi wrote:
>> During some CPU power modes entered during idle, hotplug and
>> suspend, peripherals located in the CPU power domain, such as
>> the GIC and VFP, may be powered down.  Add a notifier chain
>> that allows drivers for those peripherals to be notified
>> before and after they may be reset.
>
> I defer comment on this until I see what the CPU_COMPLEX_* stuff is used
> for.
>

Per previous discussions on this patch, CPU_COMPLEX_* will be renamed
CPU_CLUSTER_*, and will refer to all the gunk around the CPU that may
be shared by another CPU.  In this series, it is only used to save and
restore the GIC distributor registers, but it could be used for L2 as
well, if any platforms can agree on what needs to happen to the L2,
and when.

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

Re: [PATCH v9 00/12] use nonblock mmc requests to minimize latency

[Chris Ball]

Hi Per,

On Fri, Jul 01 2011, Per Forlin wrote:
> How significant is the cache maintenance over head?
> It depends, the eMMC are much faster now
> compared to a few years ago and cache maintenance cost more due to
> multiple cache levels and speculative cache pre-fetch. In relation the
> cost for handling the caches have increased and is now a bottle neck
> dealing with fast eMMC together with DMA.

Thanks very much, I've pushed v9 of the patchset to mmc-next for 3.1
(after fixing some merge conflicts against Adrian Hunter's recent patches
and rewording some of the commit message texts).

- Chris.
-- 
Chris Ball  
One Laptop Per Child

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Colin Cross]

On Sat, Jul 9, 2011 at 3:21 AM, Russell King - ARM Linux
 wrote:
> On Thu, Jul 07, 2011 at 04:50:16PM +0100, Lorenzo Pieralisi wrote:
>> From: Colin Cross 
>>
>> When the cpu is powered down in a low power mode, the gic cpu
>> interface may be reset, and when the cpu complex is powered
>> down, the gic distributor may also be reset.
>>
>> This patch uses CPU_PM_ENTER and CPU_PM_EXIT notifiers to save
>> and restore the gic cpu interface registers, and the
>> CPU_COMPLEX_PM_ENTER and CPU_COMPLEX_PM_EXIT notifiers to save
>> and restore the gic distributor registers.
>>
>> Signed-off-by: Colin Cross 
>
> Lost attributations and original author.  This is based in part on code
> from Gary King, according to patch 6646/1 in the patch system.

You're right, I'll make sure the attribution goes back in.

> Moreover, how now that we have genirq dealing with the suspend/restore
> issues, how much of this is actually required.  And should this be
> GIC specific or should there be a way of asking genirq to take care of
> some of this for us?
>
> We need to _reduce_ the amount of code needed to support this stuff, and
> if core code almost-but-not-quite does what we need then we need to talk
> to the maintainers of that code to see whether it can be changed.
>
> Because adding 212 lines to save and restore the state of every interrupt
> controller that we may have just isn't on.  We need this properly
> abstracted and dealt with in a generic way.

This is necessary for cpuidle states that lose the GIC registers, not
just suspend, because the GIC is in the cpu's power domain.  We could
avoid saving and restoring all the GIC registers in suspend and idle
by reusing the initialization functions, and then having the core irq
code call the unmask, set_type, and set_affinity functions on each irq
to reconfigure it, but that will be very inefficient - it will convert
each register write in the restore functions to a read-modify-write
per interrupt in that register.  Santosh is already complaining that
this commong GIC restore code will be slower than the automatic DMA to
restore the GIC registers that OMAP4 supports.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Russell King - ARM Linux]

On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:
> This is necessary for cpuidle states that lose the GIC registers, not
> just suspend, because the GIC is in the cpu's power domain.  We could
> avoid saving and restoring all the GIC registers in suspend and idle
> by reusing the initialization functions, and then having the core irq
> code call the unmask, set_type, and set_affinity functions on each irq
> to reconfigure it, but that will be very inefficient - it will convert
> each register write in the restore functions to a read-modify-write
> per interrupt in that register.  Santosh is already complaining that
> this commong GIC restore code will be slower than the automatic DMA to
> restore the GIC registers that OMAP4 supports.

Well, we need to come up with something sensible - a way of doing this
which doesn't require every interrupt controller driver (of which we as
an architecture have many) to have lots of support added.

If the current way is inefficient and is noticably so, then let's talk
to Thomas about finding a way around that - maybe having the generic
code make one suspend/resume callback per irq gc chip rather than doing
it per-IRQ.  We can then reuse the same paths for suspend/resume as for
idle state saving.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Colin Cross]

On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux
 wrote:
> On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:
>> This is necessary for cpuidle states that lose the GIC registers, not
>> just suspend, because the GIC is in the cpu's power domain.  We could
>> avoid saving and restoring all the GIC registers in suspend and idle
>> by reusing the initialization functions, and then having the core irq
>> code call the unmask, set_type, and set_affinity functions on each irq
>> to reconfigure it, but that will be very inefficient - it will convert
>> each register write in the restore functions to a read-modify-write
>> per interrupt in that register.  Santosh is already complaining that
>> this commong GIC restore code will be slower than the automatic DMA to
>> restore the GIC registers that OMAP4 supports.
>
> Well, we need to come up with something sensible - a way of doing this
> which doesn't require every interrupt controller driver (of which we as
> an architecture have many) to have lots of support added.
>
> If the current way is inefficient and is noticably so, then let's talk
> to Thomas about finding a way around that - maybe having the generic
> code make one suspend/resume callback per irq gc chip rather than doing
> it per-IRQ.  We can then reuse the same paths for suspend/resume as for
> idle state saving.
>

Are you referring to moving the gic driver to be gc chip?  Otherwise,
I don't understand your suggestion - how is callback per chip any
different than what this patch implements?  It just gets it's
notification through a cpu_pm notifier, which works in idle and
suspend, instead of a syscore op like the gc driver does.

This patch does save and restore some registers that are never
modified after init, so they don't need to be saved.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Russell King - ARM Linux]

On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote:
> On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux
>  wrote:
> > On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:
> >> This is necessary for cpuidle states that lose the GIC registers, not
> >> just suspend, because the GIC is in the cpu's power domain.  We could
> >> avoid saving and restoring all the GIC registers in suspend and idle
> >> by reusing the initialization functions, and then having the core irq
> >> code call the unmask, set_type, and set_affinity functions on each irq
> >> to reconfigure it, but that will be very inefficient - it will convert
> >> each register write in the restore functions to a read-modify-write
> >> per interrupt in that register.  Santosh is already complaining that
> >> this commong GIC restore code will be slower than the automatic DMA to
> >> restore the GIC registers that OMAP4 supports.
> >
> > Well, we need to come up with something sensible - a way of doing this
> > which doesn't require every interrupt controller driver (of which we as
> > an architecture have many) to have lots of support added.
> >
> > If the current way is inefficient and is noticably so, then let's talk
> > to Thomas about finding a way around that - maybe having the generic
> > code make one suspend/resume callback per irq gc chip rather than doing
> > it per-IRQ.  We can then reuse the same paths for suspend/resume as for
> > idle state saving.
> >
> 
> Are you referring to moving the gic driver to be gc chip?  Otherwise,
> I don't understand your suggestion - how is callback per chip any
> different than what this patch implements?  It just gets it's
> notification through a cpu_pm notifier, which works in idle and
> suspend, instead of a syscore op like the gc driver does.
> 
> This patch does save and restore some registers that are never
> modified after init, so they don't need to be saved.

The point is that we should aim to get to the point where, if an
interrupt controller supports PM, then it supports _all_ PM out the
box and doesn't require additional code for cpu idle PM vs system
suspend PM.

In other words, all we should need to do is provide genirq with a
couple of functions for 'save state' and 'restore state'.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Colin Cross]

On Sat, Jul 9, 2011 at 4:05 PM, Russell King - ARM Linux
 wrote:
> On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote:
>> On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux
>>  wrote:
>> > On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:
>> >> This is necessary for cpuidle states that lose the GIC registers, not
>> >> just suspend, because the GIC is in the cpu's power domain.  We could
>> >> avoid saving and restoring all the GIC registers in suspend and idle
>> >> by reusing the initialization functions, and then having the core irq
>> >> code call the unmask, set_type, and set_affinity functions on each irq
>> >> to reconfigure it, but that will be very inefficient - it will convert
>> >> each register write in the restore functions to a read-modify-write
>> >> per interrupt in that register.  Santosh is already complaining that
>> >> this commong GIC restore code will be slower than the automatic DMA to
>> >> restore the GIC registers that OMAP4 supports.
>> >
>> > Well, we need to come up with something sensible - a way of doing this
>> > which doesn't require every interrupt controller driver (of which we as
>> > an architecture have many) to have lots of support added.
>> >
>> > If the current way is inefficient and is noticably so, then let's talk
>> > to Thomas about finding a way around that - maybe having the generic
>> > code make one suspend/resume callback per irq gc chip rather than doing
>> > it per-IRQ.  We can then reuse the same paths for suspend/resume as for
>> > idle state saving.
>> >
>>
>> Are you referring to moving the gic driver to be gc chip?  Otherwise,
>> I don't understand your suggestion - how is callback per chip any
>> different than what this patch implements?  It just gets it's
>> notification through a cpu_pm notifier, which works in idle and
>> suspend, instead of a syscore op like the gc driver does.
>>
>> This patch does save and restore some registers that are never
>> modified after init, so they don't need to be saved.
>
> The point is that we should aim to get to the point where, if an
> interrupt controller supports PM, then it supports _all_ PM out the
> box and doesn't require additional code for cpu idle PM vs system
> suspend PM.

I agree 100%, and everything added in this patch is used for both idle
and suspend on Tegra, through a single entry point - cpu_pm notifiers.

> In other words, all we should need to do is provide genirq with a
> couple of functions for 'save state' and 'restore state'.

It's not so simple.

genirq doesn't know anything about idle.  The PM states in idle are
very SoC specific, so the SoC idle code would need to tell genirq that
the irq chip is going idle - using something like cpu_pm notiifers.
genirq would then just call the 'save state' and 'restore state'
functions, so what's the point?

The gic is very tightly bound to both a cpu and a cpu cluster.  There
are parts (the gic cpu interface) that must be saved and restored when
a single cpu powers down, and can only be accessed from that cpu.
Then there are parts (the gic distributor interface) that can only be
saved and restored when all cpus in a cluster power down, as well as
the cluster itself.  And then to make things more complicated, there
are per-cpu banked registers in the gic distributor, so no single cpu
can save and restore the entire gic distributor.  A single pair of
save restore functions is not sufficient for the gic, and putting the
complexity of save and restore for the gic into genirq, when all it
would be doing is passing through to the gic driver, seems
unnecessary.

Since the SoC cpu idle and suspend code generally ends up in the same
final function, it would call the same cpu_pm notifier for both idle
and suspend, and there is no duplication of code.

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

Re: [PATCH 03/17] ARM: gic: Use cpu pm notifiers to save gic state

[Santosh Shilimkar]

On 7/9/2011 4:05 PM, Russell King - ARM Linux wrote:

On Sat, Jul 09, 2011 at 04:01:19PM -0700, Colin Cross wrote:

On Sat, Jul 9, 2011 at 3:33 PM, Russell King - ARM Linux
  wrote:

On Sat, Jul 09, 2011 at 03:10:56PM -0700, Colin Cross wrote:

This is necessary for cpuidle states that lose the GIC registers, not
just suspend, because the GIC is in the cpu's power domain.  We could
avoid saving and restoring all the GIC registers in suspend and idle
by reusing the initialization functions, and then having the core irq
code call the unmask, set_type, and set_affinity functions on each irq
to reconfigure it, but that will be very inefficient - it will convert
each register write in the restore functions to a read-modify-write
per interrupt in that register.  Santosh is already complaining that
this commong GIC restore code will be slower than the automatic DMA to
restore the GIC registers that OMAP4 supports.


Well, we need to come up with something sensible - a way of doing this
which doesn't require every interrupt controller driver (of which we as
an architecture have many) to have lots of support added.

If the current way is inefficient and is noticably so, then let's talk
to Thomas about finding a way around that - maybe having the generic
code make one suspend/resume callback per irq gc chip rather than doing
it per-IRQ.  We can then reuse the same paths for suspend/resume as for
idle state saving.



Are you referring to moving the gic driver to be gc chip?  Otherwise,
I don't understand your suggestion - how is callback per chip any
different than what this patch implements?  It just gets it's
notification through a cpu_pm notifier, which works in idle and
suspend, instead of a syscore op like the gc driver does.

This patch does save and restore some registers that are never
modified after init, so they don't need to be saved.


The point is that we should aim to get to the point where, if an
interrupt controller supports PM, then it supports _all_ PM out the
box and doesn't require additional code for cpu idle PM vs system
suspend PM.

In other words, all we should need to do is provide genirq with a
couple of functions for 'save state' and 'restore state'.

Agreed.
But how generic irq code will know about when the interrupt controller
looses it's context without notifiers. This still depend on the SOC 
power domain partitions and hence platform code

At least for the GIC case, it seems depends on the CPU cluster low
power state.

Regards
Santosh

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

[ANNOUNCE] First Linaro 11.07 Build Released

[Zach Pfeffer]

All,

I'm happy to announce the first 11.07 build for generic Panda and
Beagle C4. Unfortunately Beagle xM doesn't boot, but Panda and Beagle
C4 look great!

https://android-build.linaro.org/builds/~linaro-android/beagle-11.07-release/
https://android-build.linaro.org/builds/~linaro-android/panda-11.07-release/

I'm using a only tracking tip approach, which should align with our
new CI push. As the tip's stabilize I'm going to advance the release.
This way people will have a bleeding edge build and a stable build.

Many, many, many thanks to John Stultz and Nicolas Pitre whose
diligent work on 2.6.39 we were able to pick up.

This release differs from 11.06 in:

Linaro 2.6.39
HDMI comes up in the best resolution (no more 640x480 for the upstream
focused build).

I don't like the term generic build so I'm calling these
upstream-focused builds.

The next jump will be to 3.0.

As always docs are at:

Image Install
https://wiki.linaro.org/Platform/Android/ImageInstallation

Other Docs
https://wiki.linaro.org/Platform/Android
https://wiki.linaro.org/Platform/Android/GetSource
https://wiki.linaro.org/Platform/Android/BuildSource

These builds can be tried immediately with:

Panda:

wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_panda-11.07-release/1/artifact/build/out/target/product/pandaboard/boot.tar.bz2
wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_panda-11.07-release/1/artifact/build/out/target/product/pandaboard/system.tar.bz2
wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_panda-11.07-release/1/artifact/build/out/target/product/pandaboard/userdata.tar.bz2
bzr branch lp:linaro-image-tools
./linaro-image-tools/linaro-android-media-create --mmc /dev/sdd --dev
panda --system system.tar.bz2 --userdata userdata.tar.bz2 --boot
boot.tar.bz2

Beagle

wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_beagle-11.07-release/1/artifact/build/out/target/product/beagleboard/boot.tar.bz2
wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_beagle-11.07-release/1/artifact/build/out/target/product/beagleboard/system.tar.bz2
wget --no-check-certificate
https://android-build.linaro.org/jenkins/job/linaro-android_beagle-11.07-release/1/artifact/build/out/target/product/beagleboard/userdata.tar.bz2
bzr branch lp:linaro-image-tools
./linaro-image-tools/linaro-android-media-create --mmc /dev/sdd --dev
beagle --system system.tar.bz2 --userdata userdata.tar.bz2 --boot
boot.tar.bz2

-Zach

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